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

docs: Replace non-portable "sed -i" in Makefile #27122

Merged
merged 1 commit into from Aug 1, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jul 27, 2023

We recently added a call to sed in Documentation's Makefile, as part of the generation process for the Helm reference. We use the -i option to edit the file in-place; but this option is not portable, and the command fails when running BSD versions of sed, for example on MacOS, as opposed to GNU sed.

Let's fix the command by using an additional temporary file.

Fixes: 2e9b20f ("docs: Ignore Helm value names for spellcheck")

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jul 27, 2023
@qmonnet qmonnet requested a review from a team as a code owner July 27, 2023 19:46
@qmonnet qmonnet requested a review from learnitall July 27, 2023 19:46
@joestringer
Copy link
Member

joestringer commented Jul 27, 2023

Heads up that Makefile.defs defines SED as gsed for platforms that don't have a GNU sed by default. We should probably be trying to replace usage of sed with $(SED). That said, I'm not against using more portable expressions 👍

@qmonnet
Copy link
Member Author

qmonnet commented Jul 27, 2023

I didn't know that. OK I'll update with $(SED), but I can also keep the temp file to avoid using -i anyway.

We recently added a call to "sed" in Documentation's Makefile, as part
of the generation process for the Helm reference. We use the "-i" option
to edit the file in-place; but this option is not portable, and the
command fails when running BSD versions of sed, for example on MacOS, as
opposed to GNU sed.

We have two options to fix this: using $(SED), which is set to gsed when
available, and using an additional temporary file. Let's use both to
have the Makefile as portable as possible.

Fixes: 2e9b20f ("docs: Ignore Helm value names for spellcheck")
Reported-by: Liz Rice <liz@lizrice.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet requested a review from a team as a code owner July 27, 2023 23:33
@qmonnet qmonnet requested a review from jibi July 27, 2023 23:33
@qmonnet qmonnet added needs-backport/1.12 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.1 Jul 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.6 Jul 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.13 Jul 27, 2023
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Looks good! Just a heads up though, I'm on Linux and haven't tested BSD or Mac OS.

@qmonnet
Copy link
Member Author

qmonnet commented Aug 1, 2023

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 1, 2023
@qmonnet qmonnet merged commit 685439c into cilium:main Aug 1, 2023
59 checks passed
@qmonnet qmonnet deleted the pr/sed-not-inplace branch August 1, 2023 09:43
@sayboras sayboras mentioned this pull request Aug 3, 2023
11 tasks
@sayboras sayboras added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.1 Aug 3, 2023
@sayboras sayboras mentioned this pull request Aug 3, 2023
4 tasks
@sayboras sayboras added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Aug 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.6 Aug 3, 2023
@sayboras sayboras mentioned this pull request Aug 3, 2023
1 task
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.13 Aug 3, 2023
@qmonnet qmonnet added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Aug 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.13 Aug 3, 2023
@sayboras sayboras added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.1 Aug 4, 2023
@sayboras sayboras added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Aug 4, 2023
@nebril nebril moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.6 Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.12.13
Backport done to v1.12
1.13.6
Backport done to v1.13
1.14.1
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

5 participants