Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Add masOS compatibility for 'sed' usage in scripts #1827

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Add masOS compatibility for 'sed' usage in scripts #1827

merged 1 commit into from
Jan 10, 2024

Conversation

PhilipSchmid
Copy link
Collaborator

No description provided.

@PhilipSchmid PhilipSchmid requested a review from a team as a code owner December 1, 2023 11:11
@PhilipSchmid PhilipSchmid requested a review from olsajiri December 1, 2023 11:11
@mtardy mtardy added the release-note/misc This PR makes changes that have no direct user impact. label Dec 1, 2023
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from 974818e to 8015f73 Compare December 1, 2023 12:35
@PhilipSchmid
Copy link
Collaborator Author

PhilipSchmid commented Dec 1, 2023 •
edited
Loading

Something is not happy yet with the change inside api/export-doc.sh: https://github.com/cilium/tetragon/actions/runs/7060072541/job/19218900578?pr=1827#step:5:571 ✅

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from 8015f73 to 383eee6 Compare December 1, 2023 13:04
@hungran
Copy link
Contributor

hungran commented Dec 4, 2023 •
edited
Loading

Not sure but could endup this by replace gnu-sed
Turn out me think about update document of development prequiste instead

@PhilipSchmid
Copy link
Collaborator Author

@hungran Exactly. gnu-sed would have been an option but this adds an external dependency for all macOS users and we'd like to avoid that as much as possible.

@hungran
Copy link
Contributor

hungran commented Dec 4, 2023

@hungran Exactly. gnu-sed would have been an option but this adds an external dependency for all macOS users and we'd like to avoid that as much as possible.

Makesense, wish that it already here for my PR #1797 so that it could take me confuse less 😅
Anyway +1 for this pr

mtardy
mtardy approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks!

@mtardy mtardy removed the request for review from olsajiri December 4, 2023 10:48
@mtardy mtardy added area/documentation Improvements or additions to documentation area/ci Related to CI labels Dec 4, 2023
mtardy
mtardy reviewed Dec 4, 2023
View reviewed changes
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from 383eee6 to e6dec28 Compare December 4, 2023 12:58
@netlify Netlify
Copy link

netlify bot commented Dec 4, 2023 •
edited
Loading

✅ Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit cc7ed7a
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/659ec6f38becff000860de96
😎 Deploy Preview https://deploy-preview-1827--tetragon.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.

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch 2 times, most recently from defa685 to 89749c9 Compare December 4, 2023 13:07
@mtardy
Copy link
Member

mtardy commented Dec 4, 2023

empty line issue, just run the script locally and push!

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch 3 times, most recently from 1ce148e to 6c6d5dd Compare December 5, 2023 09:02
mtardy
mtardy requested changes Dec 5, 2023
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

previous

# cleanup unecessary consecutive whitelines
sed -i '/^[[:space:]]*$/N;/^[[:space:]]*\n[[:space:]]*$/D' $TMP_FILE

was working on macOS and Linux for me.

It doesn't work for you? can you dump your sed version maybe?

@PhilipSchmid
Copy link
Collaborator Author

PhilipSchmid commented Dec 5, 2023 •
edited
Loading 

# cleanup unecessary consecutive whitelines
sed -i'' -e '/^[[:space:]]*$/N;/^[[:space:]]*\n[[:space:]]*$/D' $TMP_FILE

It works and does what it should do (cleanup unecessary consecutive whitelines) but there's no empty line at the end of the docs/content/en/docs/reference/helm-chart.md file on macOS, while there is one on Linux. Hence, I'm still searching for a solution that produces the same output for both.

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch 4 times, most recently from 4dc0ed9 to cca66f7 Compare December 5, 2023 16:54
@PhilipSchmid PhilipSchmid requested a review from mtardy December 5, 2023 20:04
mtardy
mtardy requested changes Dec 13, 2023
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

this doesn't run the same in the CI and on your platform since git status returns something. You can't reproduce?

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from cca66f7 to 9d9260f Compare December 13, 2023 12:05
@PhilipSchmid
Copy link
Collaborator Author

PhilipSchmid commented Dec 13, 2023 •
edited
Loading

Rebased to main.

@mtardy Exactly, I'm not able to reproduce this locally. cd install/kubernetes && ./test.sh doesn't produce any changes on my macOS or my Linux server 🤷‍♂️.

image

@jrfastab
Copy link
Contributor

jrfastab commented Jan 8, 2024

going to give this a small nudge. Would be nice if MacOS works also.

@PhilipSchmid PhilipSchmid requested a review from mtardy January 8, 2024 20:37
mtardy
mtardy approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks Philip, sorry for the length but those kinds of compatibility things are a bit annoying as you saw 😛!

I think the generated file test passes now, don't know what changed but it looks good! Let's merge this when the whole CI passes.

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from 9d9260f to 2db96d6 Compare January 10, 2024 16:28
@PhilipSchmid
Add masOS compatibility for 'sed' usage in scripts
cc7ed7a 
Signed-off-by: Philip Schmid <philip.schmid@isovalent.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/fix_sed_usage_for_macos branch from 2db96d6 to cc7ed7a Compare January 10, 2024 16:33
@PhilipSchmid PhilipSchmid requested a review from mtardy January 10, 2024 17:15
@mtardy
Copy link
Member

mtardy commented Jan 10, 2024

Thanks!! 🎉

@mtardy mtardy merged commit a7ec419 into cilium:main Jan 10, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mtardy mtardy Awaiting requested review from mtardy

Assignees
No one assigned
Labels
area/ci Related to CI area/documentation Improvements or additions to documentation release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PhilipSchmid @hungran @mtardy @jrfastab