Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating the Makefile script on the website to work natively with MacOS. #1417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pedroignacio13
Copy link
Contributor

After having some issues setting up the environment locally on MacOS, we figured it out what the problem was.
This PR adapts the Makefile script to work correctly when setting up the environment for local development.

Thanks Brandt and Eddie for the help on this issue!

Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for tag-security ready!

Name Link
🔨 Latest commit da862fe
🔍 Latest deploy log https://app.netlify.com/sites/tag-security/deploys/674fb90c058067000835e945
😎 Deploy Preview https://deploy-preview-1417--tag-security.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pedroignacio13 pedroignacio13 force-pushed the PedroIgnacio-Makefile-correction branch from a13f233 to da862fe Compare December 4, 2024 02:06
@mrcdb
Copy link
Member

mrcdb commented Dec 4, 2024

I am not sure if this is run in CI as well. If so, wouldn't it have an impact as the "new" sed instruction would always run? I am wondering if there would be a better way of running the script for MacOS vs Linux (e.g. with a flag or something like that)

@pedroignacio13
Copy link
Contributor Author

I'll let this PR open and try to find alternatives to this.
Let me know if you have something on your mind.

Copy link
Collaborator

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mrcdb - this doesn't work with linux (and the expected CI) processes natively.

Until such a time where this workflow is refactored - we may want to consider a conditional for macos/linux OR requiring gnu-sed on macos development environments.

Copy link
Collaborator

@eddie-knight eddie-knight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested changes will modify this to just being a comment so that it has the necessary info, but still defaults to working for CI builds

@pedroignacio13

@@ -14,14 +14,15 @@ deps:

# Update all imported markdown files to work as standalone hugo pages (except READMEs, see below)
# sed command is configured for the Netlify ubuntu env
# If you're NOT running this on a mac, you'll need to remove the empty string after -i (line 25)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# If you're NOT running this on a mac, you'll need to remove the empty string after -i (line 25)
# If you're running this on a mac, you'll need to add double quotes after -i in the sed command below: `sed -i "" "1s/^/$$text_to_prepend/" "$$file"; \`

find root -type f -name '*.md' | while IFS= read -r file; do \
base_name=$$(basename "$$file" .md); \
if [ "$$file" = "/_index.md" -o "$$base_name" = "README" ]; then \
continue; \
fi; \
title_case=$$(echo "$$base_name" | sed -e 's/-/ /g' -e 's/\b\(.\)/\u\1/g' | sed 's/cncf/CNCF/Ig'); \
text_to_prepend="---\ntitle: \"$$title_case\"\n---\n"; \
sed -i "1s/^/$$text_to_prepend/" "$$file"; \
sed -i "" "1s/^/$$text_to_prepend/" "$$file"; \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sed -i "" "1s/^/$$text_to_prepend/" "$$file"; \
sed -i "1s/^/$$text_to_prepend/" "$$file"; \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants