Fix macOS sed compatibility across all shell scripts#746
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds a portable ChangesBSD/GNU
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@telco-core/configuration/compare.sh`:
- Around line 44-45: The sed commands in compare.sh use "sed -i ''" which is
incompatible with GNU sed and causes the delete pattern to be misinterpreted;
update the two sed invocations that target source_file and rendered_file (the
lines using sed -i '' "/${file##*/}/d") to include the -e flag so the script
string is explicit (i.e., change those two sed calls to use -i '' -e with the
same "/${file##*/}/d" pattern) to restore correct file exclusion on both macOS
and GNU sed.
In `@telco-hub/configuration/reference-crs-kube-compare/compare.sh`:
- Around line 41-42: The sed -i '' invocations break on GNU sed (Linux); update
the two places that call sed (the lines removing "/${file##*/}/d" from
source_file and rendered_file and the same calls at lines ~68–69) to be
OS-aware: detect the OS (e.g., via uname -s or checking sed --version) and call
sed with the correct in-place flag — use sed -i'' "/${file##*/}/d" ... on
GNU/Linux and sed -i '' "/${file##*/}/d" ... on macOS (Darwin); modify both
occurrences so the script chooses the appropriate form at runtime and continue
to quote/expand ${file##*/} as before.
In
`@telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh`:
- Line 24: The sed command in get_ztp_installation.sh uses the macOS/BSD-only
in-place flag "-i ''", which breaks on GNU sed and causes the substitution to be
skipped; update the script to perform a portable in-place edit by detecting sed
flavor or using a compatible pattern: implement a small wrapper in the script
that checks uname or tries a test sed -i '' call and then runs sed with either
"-i ''" (BSD) or "-i" (GNU) accordingly, and apply that wrapper to the current
substitution targeting ztp-installation/argocd-openshift-gitops-patch.json (the
existing sed invocation that replaces quay.io/...:latest with
registry.redhat.io/...:v4.21).
In `@telco-ran/configuration/hack/update_file_references.sh`:
- Around line 70-75: The script aborts on GNU sed because the two uses of the
BSD-only "sed -i ''" (in update_file_references.sh around the blocks using sed
to replace paths) return a non-zero error under set -euo pipefail; add a
portable in-place-edit wrapper function (e.g., sedi) near the top of the script
after the existing "set -euo pipefail" that detects GNU vs BSD sed and invokes
sed with the correct -i syntax, then replace both occurrences of "sed -i '' -e"
in the file (the two lines that currently run sed to rewrite source-crs paths)
with calls to "sedi -e" so the script works on both macOS and Linux without
failing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5a31f2f4-983f-4c7f-8c72-a5c65b5aae96
📒 Files selected for processing (4)
telco-core/configuration/compare.shtelco-hub/configuration/reference-crs-kube-compare/compare.shtelco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.shtelco-ran/configuration/hack/update_file_references.sh
|
/retest |
Add a sedi() wrapper function to all shell scripts that use in-place sed editing. BSD sed (macOS) requires a separate empty-string backup argument (sed -i '' 's/...'), while GNU sed (Linux) treats the empty string as a separate argument and fails. The wrapper detects GNU vs BSD sed at runtime and calls the correct form.
sebrandon1
force-pushed
the
fix-macos-sed-compat
branch
from
1ff2098 to
abe971f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@telco-core/configuration/compare.sh`:
- Line 3: The one-line sedi() uses the unsafe A && B || C pattern so the macOS
fallback (sed -i '') runs when the GNU sed command itself fails; change sedi to
use an explicit if/else: run the feature check (sed --version | grep -q GNU) and
if it succeeds invoke sed -i "$@"; otherwise invoke sed -i '' "$@"; do not chain
with || so the fallback only runs when the check fails, and propagate/return the
sed command's exit status from sedi (referencing the sedi function name).
In `@telco-hub/configuration/reference-crs-kube-compare/compare.sh`:
- Line 3: The sedi() helper currently uses the fragile "&& ||" pattern which
causes the BSD branch to run even if the GNU branch fails; replace that control
flow with an explicit conditional: test sed --version output (e.g., grep -q GNU)
and, in an if/then/else, call the appropriate command (GNU: sed -i "$@"; BSD:
sed -i '' "$@") and propagate the exit status from the sed invocation so
failures are not masked; update the sedi() function to use this explicit if/else
and ensure it returns the sed exit code.
In
`@telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh`:
- Line 3: The sedi helper currently uses a chained &&/|| expression that can
mask failures; replace that one-liner with an explicit if-then-else that checks
whether GNU sed is available (using `sed --version` piped to `grep -q GNU`),
then runs the appropriate inline-edit invocation (`sed -i "$@"` for GNU or `sed
-i '' "$@"` for BSD) and captures and returns its exit status; ensure the branch
prints an error to stderr and exits/returns non-zero when the chosen sed
invocation fails so callers of the function (e.g., the code that patches
manifests) do not proceed silently on failure.
In `@telco-ran/configuration/hack/update_file_references.sh`:
- Line 15: The sedi() helper currently uses a fragile `cmd && A || B` idiom that
masks failures under `set -e`; replace it with an explicit if/else that tests
GNU sed support (run `sed --version` and check its exit status) and then runs
either `sed -i "$@"` for GNU or `sed -i '' "$@"` for BSD, ensuring any failure
from the chosen sed invocation is returned (no swallowing via `||`); update the
function name sedi() accordingly so callers keep behavior but now propagate
errors correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 71358c02-7c5b-43f5-97c7-50ec4beecdfe
📒 Files selected for processing (4)
telco-core/configuration/compare.shtelco-hub/configuration/reference-crs-kube-compare/compare.shtelco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.shtelco-ran/configuration/hack/update_file_references.sh
| @@ -1,5 +1,7 @@ | |||
| #! /bin/bash | |||
|
|
|||
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n telco-core/configuration/compare.sh | head -20Repository: openshift-kni/telco-reference
Length of output: 565
sedi() fallback executes even when GNU sed -i fails, masking errors (Line 3).
The A && B || C pattern is unsafe: if sed -i "$@" fails, the || sed -i '' "$@" still executes, hiding the original failure and potentially causing nondeterministic behavior. Use explicit if-else branching to ensure the fallback only runs when the condition check fails, not when the command fails.
Suggested fix
-sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
+sedi() {
+ if sed --version 2>/dev/null | grep -q GNU; then
+ sed -i "$@"
+ else
+ sed -i '' "$@"
+ fi
+}🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 3-3: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@telco-core/configuration/compare.sh` at line 3, The one-line sedi() uses the
unsafe A && B || C pattern so the macOS fallback (sed -i '') runs when the GNU
sed command itself fails; change sedi to use an explicit if/else: run the
feature check (sed --version | grep -q GNU) and if it succeeds invoke sed -i
"$@"; otherwise invoke sed -i '' "$@"; do not chain with || so the fallback only
runs when the check fails, and propagate/return the sed command's exit status
from sedi (referencing the sedi function name).
There was a problem hiding this comment.
@sebrandon1 please review/consider this suggestion. Thanks!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -1,5 +1,7 @@ | |||
| #! /bin/bash | |||
|
|
|||
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n telco-hub/configuration/reference-crs-kube-compare/compare.sh | head -10Repository: openshift-kni/telco-reference
Length of output: 331
sedi() control flow allows silent failure masking (Line 3).
The && || pattern will execute the BSD branch even if the GNU branch fails, obscuring the real error. When sed -i "$@" fails, the fallback sed -i '' "$@" runs anyway instead of propagating the failure.
Suggested fix
-sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
+sedi() {
+ if sed --version 2>/dev/null | grep -q GNU; then
+ sed -i "$@"
+ else
+ sed -i '' "$@"
+ fi
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |
| sedi() { | |
| if sed --version 2>/dev/null | grep -q GNU; then | |
| sed -i "$@" | |
| else | |
| sed -i '' "$@" | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 3-3: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@telco-hub/configuration/reference-crs-kube-compare/compare.sh` at line 3, The
sedi() helper currently uses the fragile "&& ||" pattern which causes the BSD
branch to run even if the GNU branch fails; replace that control flow with an
explicit conditional: test sed --version output (e.g., grep -q GNU) and, in an
if/then/else, call the appropriate command (GNU: sed -i "$@"; BSD: sed -i ''
"$@") and propagate the exit status from the sed invocation so failures are not
masked; update the sedi() function to use this explicit if/else and ensure it
returns the sed exit code.
| @@ -1,5 +1,7 @@ | |||
| #! /bin/bash | |||
|
|
|||
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file in the repository
find . -path "*telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh"Repository: openshift-kni/telco-reference
Length of output: 153
🏁 Script executed:
# Read the first 20 lines of the file to see the code and context
head -20 ./telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.shRepository: openshift-kni/telco-reference
Length of output: 1192
🏁 Script executed:
# Count usages of sedi function in the file
grep -n "sedi " ./telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh | head -20Repository: openshift-kni/telco-reference
Length of output: 247
🏁 Script executed:
# Count all occurrences of sedi (function definition + calls)
grep -c "sedi" ./telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh
# Get full context around the sedi usage
grep -n -B 2 -A 2 "sedi " ./telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.shRepository: openshift-kni/telco-reference
Length of output: 386
🏁 Script executed:
# Verify the && || pattern behavior to confirm the issue
# Create a test to show the problem
cat <<'EOF' > /tmp/test_pattern.sh
#!/bin/bash
# Test the problematic pattern
test_pattern() {
# Simulate a failing command in position B of "A && B || C"
(exit 0) && (exit 1) || echo "C was executed (error masked)"
# Verify with explicit if-then-else
if (exit 0); then
(exit 1) && echo "This won't print"
else
echo "This won't print either"
fi
}
test_pattern
EOF
bash /tmp/test_pattern.shRepository: openshift-kni/telco-reference
Length of output: 103
Use explicit if-then-else instead of && || to prevent masking sed failures (Line 3).
The pattern sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@" masks errors: if sed -i fails, the fallback executes silently without signaling the failure. Since this function patches critical manifest versions at line 26, undetected sed failures could result in wrong deployment versions.
Suggested fix
-sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
+sedi() {
+ if sed --version 2>/dev/null | grep -q GNU; then
+ sed -i "$@"
+ else
+ sed -i '' "$@"
+ fi
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |
| sedi() { | |
| if sed --version 2>/dev/null | grep -q GNU; then | |
| sed -i "$@" | |
| else | |
| sed -i '' "$@" | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 3-3: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh`
at line 3, The sedi helper currently uses a chained &&/|| expression that can
mask failures; replace that one-liner with an explicit if-then-else that checks
whether GNU sed is available (using `sed --version` piped to `grep -q GNU`),
then runs the appropriate inline-edit invocation (`sed -i "$@"` for GNU or `sed
-i '' "$@"` for BSD) and captures and returns its exit status; ensure the branch
prints an error to stderr and exits/returns non-zero when the chosen sed
invocation fails so callers of the function (e.g., the code that patches
manifests) do not proceed silently on failure.
|
|
||
| set -euo pipefail | ||
|
|
||
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find it
fd update_file_references.shRepository: openshift-kni/telco-reference
Length of output: 128
🏁 Script executed:
cat -n telco-ran/configuration/hack/update_file_references.shRepository: openshift-kni/telco-reference
Length of output: 3790
sedi() uses a non-safe conditional idiom under set -e (Line 15).
Under set -euo pipefail, the && ... || ... construct can mask sed failures. If sed -i "$@" fails after GNU sed is detected, the || operator silently executes the fallback, preventing proper error detection and exit. This is particularly problematic since the script modifies files in-place and requires reliable error reporting.
Use explicit if/else to preserve correct error behavior:
Suggested fix
-sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
+sedi() {
+ if sed --version 2>/dev/null | grep -q GNU; then
+ sed -i "$@"
+ else
+ sed -i '' "$@"
+ fi
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; } | |
| sedi() { | |
| if sed --version 2>/dev/null | grep -q GNU; then | |
| sed -i "$@" | |
| else | |
| sed -i '' "$@" | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 15-15: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@telco-ran/configuration/hack/update_file_references.sh` at line 15, The
sedi() helper currently uses a fragile `cmd && A || B` idiom that masks failures
under `set -e`; replace it with an explicit if/else that tests GNU sed support
(run `sed --version` and check its exit status) and then runs either `sed -i
"$@"` for GNU or `sed -i '' "$@"` for BSD, ensuring any failure from the chosen
sed invocation is returned (no swallowing via `||`); update the function name
sedi() accordingly so callers keep behavior but now propagate errors correctly.
3 participants
Summary
sedi()wrapper function to all 4 shell scripts that use in-place sed editingsed -iinvocations withsedicallsProblem
BSD sed (macOS) requires
sed -i '' 's/...'(empty string as separate argument), while GNU sed (Linux) requiressed -i 's/...'(no separate argument). Using either form alone breaks the other platform. The previoussed -icalls worked on Linux CI but failed on macOS. Simply changing tosed -i ''(as attempted in the original version of this PR) fixes macOS but breaks Linux.Solution
A one-line wrapper detects sed flavor at runtime:
Files fixed
telco-core/configuration/compare.sh(2 sed calls)telco-hub/configuration/reference-crs-kube-compare/compare.sh(4 sed calls)telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh(1 sed call)telco-ran/configuration/hack/update_file_references.sh(2 sed calls)