Skip to content

Fix macOS sed compatibility across all shell scripts #746

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.

Sign up for GitHub

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

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

Fix macOS sed compatibility across all shell scripts #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions telco-core/configuration/compare.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#! /bin/bash

sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

cat -n telco-core/configuration/compare.sh | head -20

Repository: 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).

Copy link
Copy Markdown
Collaborator

@imiller0 imiller0 May 18, 2026

Choose a reason for hiding this comment

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

@sebrandon1 please review/consider this suggestion. Thanks!

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

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

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!


trap cleanup EXIT

function cleanup() {
Expand Down Expand Up @@ -41,8 +43,8 @@ function compare_cr {
while IFS= read -r file; do
[[ ${file::1} != "#" ]] || continue # Skip any comment lines in the exclusionfile
[[ -n ${file} ]] || continue # Skip empty lines
sed -i "/${file##*/}/d" source_file
sed -i "/${file##*/}/d" rendered_file
sedi "/${file##*/}/d" source_file
sedi "/${file##*/}/d" rendered_file
done < <(cat same_file "$exclusionfile")

if [[ -s source_file || -s rendered_file ]]; then
Expand Down
10 changes: 6 additions & 4 deletions telco-hub/configuration/reference-crs-kube-compare/compare.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#! /bin/bash

sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

cat -n telco-hub/configuration/reference-crs-kube-compare/compare.sh | head -10

Repository: 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.

Suggested change
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.


trap cleanup EXIT

function cleanup() {
Expand Down Expand Up @@ -38,8 +40,8 @@ function compare_cr {
while IFS= read -r file; do
[[ ${file::1} != "#" ]] || continue # Skip any comment lines in the exclusionfile
[[ -n ${file} ]] || continue # Skip empty lines
sed -i "/${file##*/}/d" source_file
sed -i "/${file##*/}/d" rendered_file
sedi "/${file##*/}/d" source_file
sedi "/${file##*/}/d" rendered_file
done < "$exclusionfile"

local source_cr rendered
Expand All @@ -65,8 +67,8 @@ function compare_cr {
while IFS= read -r file; do
[[ ${file::1} != "#" ]] || continue # Skip any comment lines in the exclusionfile
[[ -n ${file} ]] || continue # Skip empty lines
sed -i "/${file##*/}/d" source_file
sed -i "/${file##*/}/d" rendered_file
sedi "/${file##*/}/d" source_file
sedi "/${file##*/}/d" rendered_file
done < <(cat same_file "$exclusionfile")

if [[ -s source_file || -s rendered_file ]]; then
Expand Down
4 changes: 3 additions & 1 deletion telco-hub/configuration/reference-crs/required/gitops/get_ztp_installation.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#! /bin/bash

sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.sh

Repository: 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 -20

Repository: 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.sh

Repository: 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.sh

Repository: 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.

Suggested change
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.


# This script is only needed to create the ztp-installation manifest
# once per each Minor version.

Expand All @@ -21,7 +23,7 @@ find ./ztp-installation/ -name "*.yaml" -exec yq -i eval '.metadata.annotations.

# patch the ztp-site-generate version
echo " - Patch ztp-site-generate version"
sed -i 's|quay.io/openshift-kni/ztp-site-generator:latest|registry.redhat.io/openshift4/ztp-site-generate-rhel8:v4.21|g' ztp-installation/argocd-openshift-gitops-patch.json
sedi 's|quay.io/openshift-kni/ztp-site-generator:latest|registry.redhat.io/openshift4/ztp-site-generate-rhel8:v4.21|g' ztp-installation/argocd-openshift-gitops-patch.json

echo " - Adding elements to the whitelist"
yq '.spec.namespaceResourceWhitelist += {"group": "'metal3.io'", "kind": "DataImage"}' ztp-installation/app-project.yaml
Expand Down
6 changes: 4 additions & 2 deletions telco-ran/configuration/hack/update_file_references.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

set -euo pipefail

sedi() { sed --version 2>/dev/null | grep -q GNU && sed -i "$@" || sed -i '' "$@"; }
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and find it
fd update_file_references.sh

Repository: openshift-kni/telco-reference

Length of output: 128

🏁 Script executed:

cat -n telco-ran/configuration/hack/update_file_references.sh

Repository: 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.

Suggested change
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.


if [[ ! -d "source-crs" ]]; then
echo "Error: This script must be run from a directory containing 'source-crs' (e.g., the 'configuration' directory)." >&2
exit 1
Expand Down Expand Up @@ -67,12 +69,12 @@ for source_file in "${source_files[@]}"; do
# Choose the replacement strategy based on the file's content.
if grep -q "source-crs/.*$file_name" "$target_file"; then
# This file contains an incorrect 'source-crs' path. Fix it.
sed -i -e "s|source-crs/[^[:space:]\`[]*${escaped_file_name}|source-crs/$replacement_path|g" "$target_file"
sedi -e "s|source-crs/[^[:space:]\`[]*${escaped_file_name}|source-crs/$replacement_path|g" "$target_file"
else
# This file contains a different incorrect full path. Fix it.
# This regex finds a path-like string ending in the filename.
replacement_path=${replacement_path#%source-crs/}
sed -i -e "s|[^[:space:]\`[,]*${escaped_file_name}|${replacement_path}|g" "$target_file"
sedi -e "s|[^[:space:]\`[,]*${escaped_file_name}|${replacement_path}|g" "$target_file"
fi
done
done
Expand Down
Loading