Ops 5780 cap 1 ci to build and publish arm based docker images#36
Ops 5780 cap 1 ci to build and publish arm based docker images#36zeevkamin-mend wants to merge 15 commits into
Conversation
- Move ARM64 fixes to scanner-full-modifications.txt (only for Dockerfile.full) - Keep scanner-modifications.txt clean (only Mono/Nuget, no ARM64 fixes) - Update copy.sh to use scanner-full config for Dockerfile.full - Fixes 'unknown instruction: wget' error in regular scanner Dockerfile
📝 WalkthroughWalkthroughReworks the staging build-and-push workflow to run a matrix over architectures (amd64, arm64) with per-arch runner_profile, tag_suffix, platform_info, and handle_git controls. Updates checkout to use the actual Git ref, splits build and publish into per-arch steps that pass TAG_SUFFIX to build/publish scripts, and conditions git operations on matrix.handle_git. Adds TAG_SUFFIX/platform parameters to bin/build.sh, bin/publish.sh, and bin/notify-slack.sh; adjusts bin/copy.sh and bin/modify-dockerfile.sh for Dockerfile edits; introduces architecture-aware modification configs for scanner-full and remediate Dockerfiles. Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/publish.sh (1)
10-10:⚠️ Potential issue | 🟡 MinorStale usage message — omits the optional
TAG_SUFFIXargument.📝 Proposed fix
- echo "Usage: $0 <release> <registry_prefix>" + echo "Usage: $0 <release> <registry_prefix> [tag_suffix]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/publish.sh` at line 10, The usage message in bin/publish.sh currently prints only echo "Usage: $0 <release> <registry_prefix>" and is missing the optional TAG_SUFFIX parameter; update that echo call in publish.sh to include the optional argument (e.g., append [TAG_SUFFIX] to the usage string) so the script usage reflects all accepted parameters.
🧹 Nitpick comments (2)
bin/notify-slack.sh (1)
15-18:PLATFORM_TAG_SUFFIXis hardcoded to-arm64for any non-emptyPLATFORM_INFO, independent of its actual value.This creates a silent coupling: if a caller ever passes a non-ARM64
PLATFORM_INFO, the Slack message will still display-arm64tags that don't match what was actually pushed.build.shandpublish.shalready acceptTAG_SUFFIXdirectly — passing it here as a 5th argument would keep all three scripts in sync and remove the implicit assumption.♻️ Proposed refactor
PLATFORM_INFO=$4 # Optional: "ARM64" or empty +TAG_SUFFIX=$5 # Optional: "-arm64" or empty; mirrors build.sh/publish.sh # Set platform prefix and tag suffix if platform info provided PLATFORM_PREFIX="" -PLATFORM_TAG_SUFFIX="" if [ -n "$PLATFORM_INFO" ]; then PLATFORM_PREFIX="[${PLATFORM_INFO}] " - PLATFORM_TAG_SUFFIX="-arm64" fi +PLATFORM_TAG_SUFFIX="${TAG_SUFFIX}"And update the workflow call to pass
${{ matrix.tag_suffix }}as the fifth argument.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/notify-slack.sh` around lines 15 - 18, The script sets PLATFORM_TAG_SUFFIX to the literal "-arm64" whenever PLATFORM_INFO is non-empty, which is incorrect; update the logic to accept a TAG_SUFFIX argument instead of hardcoding so PLATFORM_TAG_SUFFIX reflects the actual tag passed in (consistent with build.sh and publish.sh). Modify bin/notify-slack.sh to read a fifth positional parameter (e.g., TAG_SUFFIX) and set PLATFORM_TAG_SUFFIX="$TAG_SUFFIX" when PLATFORM_INFO is present, preserve PLATFORM_PREFIX="[${PLATFORM_INFO}] " behavior, and update your workflow/caller to pass matrix.tag_suffix as that fifth argument so all three scripts use the same tag suffix..github/workflows/stg-from-version-build-push-tag-base-image.yaml (1)
160-167: Slack notifications are fully commented out — no deployment notifications will be sent.The previous active Slack step has been replaced entirely by a commented-out block. If suppression is only temporary (e.g., to stabilise the new matrix flow), track this with a TODO or a follow-up issue so it isn't forgotten.
Would you like me to open a follow-up issue to re-enable Slack notifications once the multi-arch pipeline is stable?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml around lines 160 - 167, Slack notification step ("Notify Slack - Images Ready (${{ matrix.arch }})") is fully commented out and will never run; either re-enable it behind a stable conditional or add explicit tracking so it isn't forgotten: uncomment the step that calls ./bin/notify-slack.sh and restore the if: condition (if: always() && github.event.inputs.TriggeredBy != 'ubuntu-refresh') or, if you want it suppressed temporarily, add a clear TODO comment above the block and create a follow-up issue referencing the commented step name and the ./bin/notify-slack.sh invocation so the team remembers to re-enable notifications once the matrix flow is stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml:
- Around line 39-48: The matrix currently sets handle_git: false for both
entries so the workflow condition if: matrix.handle_git == true never runs;
update the matrix so only one architecture (typically the amd64 entry with arch:
amd64 and runner_profile: developer-platform-xlarge-od) has handle_git: true and
leave the arm64 entry (arch: arm64) with handle_git: false so
Branch/Commit/Push/Tag steps guarded by matrix.handle_git run exactly once.
- Around line 91-92: Remove the temporary debug step named "sleep debug" that
runs "sleep 999999" so matrix jobs no longer stall; delete the entire step block
or replace it with a non-blocking conditional (e.g., run only when a debug flag
is set) or a short, bounded sleep if absolutely needed, ensuring no
unconditional long-running "sleep 999999" remains in the workflow.
- Line 52: The workflow uses an outdated checkout action version; update the
uses reference "uses: actions/checkout@v3" to the maintained Node-20-based v4
(e.g., "uses: actions/checkout@v4" or pin to "actions/checkout@v4.3.0") so the
job no longer runs on the deprecated Node 16 runtime.
In `@bin/modify-dockerfile.sh`:
- Line 39: The sed invocations using a hard-coded '|' delimiter (e.g., the sed
call that uses "$pattern" and "$temp_file") can break when pattern or
replacement contains '|'; change to validate/escape or pick a safe delimiter
dynamically: before the case block, inspect the variables pattern and
replacement and either escape any occurrences of the chosen delimiter (so sed
sees a valid delimiter) or choose a delimiter character not present in
pattern/replacement (fall back to a safe choice), then use that delimiter in the
sed command that operates on temp_file; apply the same validation/escaping logic
to all other sed uses referenced in the script (including the other occurrences
handling replacement and deletion).
---
Outside diff comments:
In `@bin/publish.sh`:
- Line 10: The usage message in bin/publish.sh currently prints only echo
"Usage: $0 <release> <registry_prefix>" and is missing the optional TAG_SUFFIX
parameter; update that echo call in publish.sh to include the optional argument
(e.g., append [TAG_SUFFIX] to the usage string) so the script usage reflects all
accepted parameters.
---
Nitpick comments:
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml:
- Around line 160-167: Slack notification step ("Notify Slack - Images Ready
(${{ matrix.arch }})") is fully commented out and will never run; either
re-enable it behind a stable conditional or add explicit tracking so it isn't
forgotten: uncomment the step that calls ./bin/notify-slack.sh and restore the
if: condition (if: always() && github.event.inputs.TriggeredBy !=
'ubuntu-refresh') or, if you want it suppressed temporarily, add a clear TODO
comment above the block and create a follow-up issue referencing the commented
step name and the ./bin/notify-slack.sh invocation so the team remembers to
re-enable notifications once the matrix flow is stable.
In `@bin/notify-slack.sh`:
- Around line 15-18: The script sets PLATFORM_TAG_SUFFIX to the literal "-arm64"
whenever PLATFORM_INFO is non-empty, which is incorrect; update the logic to
accept a TAG_SUFFIX argument instead of hardcoding so PLATFORM_TAG_SUFFIX
reflects the actual tag passed in (consistent with build.sh and publish.sh).
Modify bin/notify-slack.sh to read a fifth positional parameter (e.g.,
TAG_SUFFIX) and set PLATFORM_TAG_SUFFIX="$TAG_SUFFIX" when PLATFORM_INFO is
present, preserve PLATFORM_PREFIX="[${PLATFORM_INFO}] " behavior, and update
your workflow/caller to pass matrix.tag_suffix as that fifth argument so all
three scripts use the same tag suffix.
| - arch: amd64 | ||
| runner_profile: developer-platform-xlarge-od | ||
| tag_suffix: "" | ||
| platform_info: "" | ||
| handle_git: false | ||
| - arch: arm64 | ||
| runner_profile: renovate-arm-od | ||
| tag_suffix: "-arm64" | ||
| platform_info: "ARM64" | ||
| handle_git: false |
There was a problem hiding this comment.
handle_git: false for both matrix entries fully disables the Branch, Commit, Push and Tag Changes step.
Both amd64 and arm64 set handle_git: false, so the if: matrix.handle_git == true guard on Line 113 is never satisfied. Release branch creation, tagging, and merge-to-develop will never execute. Before this PR those operations ran unconditionally; this is a functional regression.
If the intent is that only one architecture performs git operations (as is typical — git work should not be duplicated per-arch), the amd64 entry should have handle_git: true:
- arch: amd64
runner_profile: developer-platform-xlarge-od
tag_suffix: ""
platform_info: ""
- handle_git: false
+ handle_git: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml around
lines 39 - 48, The matrix currently sets handle_git: false for both entries so
the workflow condition if: matrix.handle_git == true never runs; update the
matrix so only one architecture (typically the amd64 entry with arch: amd64 and
runner_profile: developer-platform-xlarge-od) has handle_git: true and leave the
arm64 entry (arch: arm64) with handle_git: false so Branch/Commit/Push/Tag steps
guarded by matrix.handle_git run exactly once.
There was a problem hiding this comment.
@nadav-t, glad we're aligned! Here's the fix to apply — only the amd64 entry should drive git operations so the release branch, tag, and merge-to-develop run exactly once:
- arch: amd64
runner_profile: developer-platform-xlarge-od
tag_suffix: ""
platform_info: ""
- handle_git: false
+ handle_git: true
- arch: arm64
runner_profile: renovate-arm-od
tag_suffix: "-arm64"
platform_info: "ARM64"
handle_git: falseThis ensures the Branch, Commit, Push and Tag Changes step (guarded by if: matrix.handle_git == true) executes exactly once — on the amd64 runner — restoring the pre-PR behaviour.
🧠 Learnings used
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : Base images must contain only open-source tools, no proprietary Mend code
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : All source Dockerfiles must have `# END OF BASE IMAGE` comment to mark truncation point
|
|
||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/checkout v4 release current version recommended 2024 2025
💡 Result:
- Latest overall
actions/checkout(as of Feb 22, 2026): v6.0.2 (released Jan 9, 2026). [1] - Latest in the
v4major line: v4.3.0 (released Aug 11, 2025). [2][3] v4major release date: v4.0.0 was released Oct 17, 2023. [3]
What’s recommended (2024–2025 and now):
- If you’re still on v4, the recommended “current v4” to target is
actions/checkout@v4(it moves to the latest v4.x, currently v4.3.0). [2][3] - For new/upgraded workflows, prefer the latest major (
actions/checkout@v6, currently v6.0.2). [1] - Security best practice (2024–2025+): pin actions to a full commit SHA (optionally with a comment like
# v4.3.0) rather than only@v4/@v6. [4] GitHub also added policy support to enforce SHA pinning. [5]
Sources:
[1] https://github.com/actions/checkout/releases/tag/v6.0.2
[2] https://github.com/actions/checkout/releases/tag/v4.3.0
[3] https://releasealert.dev/github/actions/checkout
[4] https://wellarchitected.github.com/library/application-security/recommendations/actions-security/
[5] https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
🌐 Web query:
actionlint actions/checkout v3 v4 deprecation support
💡 Result:
actions/checkout@v3isn’t “deprecated” by itself, but it runs on the Node.js 16 action runtime, and GitHub has been transitioning Actions from Node 16 to Node 20, showing warnings for workflows that still use Node16-based actions and encouraging upgrades to Node20-based major versions (likecheckout@v4). [1]actions/checkout@v4is the Node20-based major version and is the recommended upgrade target to eliminate Node16 deprecation warnings. [2]- actionlint supports workflows using
actions/checkout@v4(it’s used in actionlint’s own examples). [3] - If actionlint is complaining about
checkout@v4inputs/outputs, update actionlint—it periodically refreshes its “popular actions” metadata used for action-usage checking. Recent releases explicitly mention updating this dataset. [4]
Sources
[1] GitHub Changelog: “GitHub Actions: Transitioning from Node 16 to Node 20” (Sep 22, 2023). (github.blog)
[2] Example discussion noting checkout@v3 uses node16 and should upgrade to v4. (github.com)
[3] actionlint repo README example uses actions/checkout@v4. (github.com)
[4] actionlint releases note: “Update the popular actions data set to the latest.” (github.com)
Citations:
- 1: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/?utm_source=openai
- 2: [Github Actions] actions/checkout@v3 should be upgraded to v4 freshframework/fresh#2285
- 3: https://github.com/rhysd/actionlint?utm_source=openai
- 4: https://github.com/rhysd/actionlint/releases?utm_source=openai
Upgrade actions/checkout@v3 to v4 — v3 runs on the deprecated Node 16 runtime, while v4 is Node 20-based and currently maintained (latest v4.3.0).
- uses: actions/checkout@v3
+ uses: actions/checkout@v4📝 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.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.11)
[error] 52-52: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml at line
52, The workflow uses an outdated checkout action version; update the uses
reference "uses: actions/checkout@v3" to the maintained Node-20-based v4 (e.g.,
"uses: actions/checkout@v4" or pin to "actions/checkout@v4.3.0") so the job no
longer runs on the deprecated Node 16 runtime.
| # Remove lines matching pattern | ||
| sed -i.bak "/${pattern}/d" "$temp_file" | ||
| # Use \| syntax to specify | as delimiter and avoid conflicts with / in paths | ||
| sed -i.bak "\\|${pattern}|d" "$temp_file" |
There was a problem hiding this comment.
Unguarded | in $pattern/$replacement will break all sed commands.
Switching from / to | fixes the slash-in-path problem but introduces the symmetric risk: if any pattern or replacement value from a modifications config file contains a literal | (e.g., a regex alternation like curl|wget, or a Dockerfile line that pipes commands), sed will misparse the delimiter and either produce a wrong substitution or fail outright. With set -e active the script aborts immediately, leaving whatever CI step invoked it in a broken state.
Per coding guidelines, inputs should be validated in scripts that modify files. A minimal guard before the case block would suffice:
🛡️ Proposed guard against delimiter collision
# Parse command format: ACTION:PATTERN:REPLACEMENT
IFS=':' read -r action pattern replacement <<< "$line"
+
+ # Guard: | is used as the sed delimiter; reject patterns/replacements that contain it
+ if [[ "$pattern" == *"|"* || "$replacement" == *"|"* ]]; then
+ echo " Error: pattern or replacement contains '|' which conflicts with the sed delimiter. Skipping: $line"
+ continue
+ fi
echo " Processing: $action for pattern '$pattern'"As per coding guidelines, bin/*.sh files must "Always validate inputs, especially in scripts that modify files or deploy infrastructure."
Also applies to: 44-44, 134-135, 140-141, 146-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/modify-dockerfile.sh` at line 39, The sed invocations using a hard-coded
'|' delimiter (e.g., the sed call that uses "$pattern" and "$temp_file") can
break when pattern or replacement contains '|'; change to validate/escape or
pick a safe delimiter dynamically: before the case block, inspect the variables
pattern and replacement and either escape any occurrences of the chosen
delimiter (so sed sees a valid delimiter) or choose a delimiter character not
present in pattern/replacement (fall back to a safe choice), then use that
delimiter in the sed command that operates on temp_file; apply the same
validation/escaping logic to all other sed uses referenced in the script
(including the other occurrences handling replacement and deletion).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml:
- Around line 157-164: The Slack notification step ("Notify Slack - Images Ready
(${{ matrix.arch }})") has been fully commented out; restore or make explicit
the intended behavior by uncommenting the step (including the if: always() &&
github.event.inputs.TriggeredBy != 'ubuntu-refresh', env entries ECR_REGISTRY
and SLACK_WEBHOOK_URL, and the run block that calls ./bin/notify-slack.sh with
the Tag, job.status, run URL and matrix.platform_info), or replace it with a
no-op plus a clear TODO/issue reference so the omission isn't forgotten before
merge; ensure the restored step preserves the original condition and parameters.
---
Duplicate comments:
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml:
- Line 52: Update the GitHub Actions step that references the checkout action:
change the usage string "uses: actions/checkout@v3" to "uses:
actions/checkout@v4" so the workflow uses the Node 18+ compatible checkout
action; ensure any pinned inputs remain compatible with the v4 action.
- Around line 39-48: The matrix currently sets handle_git: false for both arch
entries so Git operations never run; update the amd64 matrix entry (the one with
arch: amd64 and runner_profile: developer-platform-xlarge-od) to set handle_git:
true so that the "Branch, Commit, Push and Tag Changes" step executes exactly
once while keeping arm64's handle_git: false; ensure only the amd64 entry is
changed.
| # - name: Notify Slack - Images Ready (${{ matrix.arch }}) | ||
| # if: always() && github.event.inputs.TriggeredBy != 'ubuntu-refresh' | ||
| # env: | ||
| # ECR_REGISTRY: ${{ steps.login-ecr.outputs.registry }} | ||
| # SLACK_WEBHOOK_URL: ${{ secrets.STG_SLACK_WEBHOOK_URL }} | ||
| # run: | | ||
| # echo "📤 Sending Slack notification for ${{ matrix.arch }}..." | ||
| # ./bin/notify-slack.sh "${{ github.event.inputs.Tag }}" "${{ job.status }}" "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" "${{ matrix.platform_info }}" |
There was a problem hiding this comment.
Slack notification is fully commented out — no build status notifications will be sent.
The previous workflow notified Slack on every run (with if: always()). With this block commented out, the team loses visibility into build successes and failures. If this is intentional for the WIP phase, consider adding a TODO or tracking issue so it isn't forgotten before merging to main.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/stg-from-version-build-push-tag-base-image.yaml around
lines 157 - 164, The Slack notification step ("Notify Slack - Images Ready (${{
matrix.arch }})") has been fully commented out; restore or make explicit the
intended behavior by uncommenting the step (including the if: always() &&
github.event.inputs.TriggeredBy != 'ubuntu-refresh', env entries ECR_REGISTRY
and SLACK_WEBHOOK_URL, and the run block that calls ./bin/notify-slack.sh with
the Tag, job.status, run URL and matrix.platform_info), or replace it with a
no-op plus a clear TODO/issue reference so the omission isn't forgotten before
merge; ensure the restored step preserves the original condition and parameters.
There was a problem hiding this comment.
@nadav-t, the concern is about losing build-status visibility for the team — the previous workflow always sent a Slack notification (success or failure) after every run. Now that it's commented out, no notifications will be sent at all.
That said, there's a likely valid reason here: with the new matrix strategy (amd64 + arm64), the notification step would fire twice per workflow trigger — once for each arch — which could be noisy or confusing. That's a reasonable motivation to temporarily suppress it while figuring out the right approach (e.g., send one aggregated notification, or only notify from the handle_git: true arch when that's eventually enabled).
The flag is simply a reminder that commenting it out entirely means no failure alerts either, which is a regression in observability. If this is intentional for the WIP phase, a short comment like # TODO: re-enable once matrix notification strategy is decided would make the intent clear to reviewers and prevent it from being forgotten before the PR merges to main.
🧠 Learnings used
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : Base images must contain only open-source tools, no proprietary Mend code
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : All source Dockerfiles must have `# END OF BASE IMAGE` comment to mark truncation point
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/modify-dockerfile.sh`:
- Line 44: In modify_dockerfile the sed expression sed -i.bak
"s@^\(.*${pattern}.*\)$@# \1@" "$temp_file" is broken because the double-quoted
string contains $@ which the shell expands; change the quoting so the sed script
is a single argument and ${pattern} is still expanded (for example use mixed
quoting: wrap the sed script in single quotes and inject the expanded ${pattern}
with "'${pattern}'" or similar), leaving "$temp_file" as a separate quoted
argument; ensure the unique symbols mentioned (modify_dockerfile and the sed
invocation) are updated so the shell never sees $@ inside the sed script.
---
Duplicate comments:
In `@bin/modify-dockerfile.sh`:
- Around line 38-146: The sed invocations using "@" as a hardcoded delimiter (in
branches handling COMMENT, COMMENT_BLOCK, ADD_AFTER, ADD_BEFORE, REPLACE) will
break if the input pattern or replacement contains "@"; update the script to
validate and sanitize delimiters before running sed: implement a helper that
examines "$pattern" and "$replacement" and selects a safe delimiter (e.g., from
a list like '@|:;#%_~') or escapes the delimiter occurrences in the inputs, then
use that delimiter in the sed -i.bak commands (and in the Python heredoc
handling of pattern) so the sed expressions in the COMMENT, ADD_AFTER,
ADD_BEFORE and REPLACE branches and the pattern matching in COMMENT_BLOCK are
robust against "@" in Dockerfile content. Ensure the helper is invoked wherever
sed is used so no sed command is executed with an unguarded delimiter under set
-e.
| # Comment out lines matching pattern - simple single line | ||
| sed -i.bak "s/^\(.*${pattern}.*\)$/# \1/" "$temp_file" | ||
| # Use @ as delimiter to avoid conflicts with / in paths and | in pipes | ||
| sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file" |
There was a problem hiding this comment.
$@ in the double-quoted sed expression corrupts the COMMENT action — SC2145 is not a false positive.
The string "s@^\(.*${pattern}.*\)$@# \1@" contains \)$@ where bash parses $@ as the "all positional parameters" special variable. Inside modify_dockerfile(), $@ is the two function arguments (Dockerfile path + commands-file path). Bash splits this into two separate words:
- word 1:
"s@^\(.*<pattern>.*\)<dockerfile_path>" - word 2:
"<commands_file_path># \1@"
sed receives two garbled arguments instead of a single valid expression, so every COMMENT action silently fails or produces wrong output.
Fix by using mixed quoting so $@ is never interpreted by the shell:
🐛 Proposed fix
- sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file"
+ sed -i.bak 's@^\(.*'"${pattern}"'.*\)$@# \1@' "$temp_file"📝 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.
| sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file" | |
| sed -i.bak 's@^\(.*'"${pattern}"'.*\)$@# \1@' "$temp_file" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 44-44: Argument mixes string and array. Use * or separate argument.
(SC2145)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/modify-dockerfile.sh` at line 44, In modify_dockerfile the sed expression
sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file" is broken because the
double-quoted string contains $@ which the shell expands; change the quoting so
the sed script is a single argument and ${pattern} is still expanded (for
example use mixed quoting: wrap the sed script in single quotes and inject the
expanded ${pattern} with "'${pattern}'" or similar), leaving "$temp_file" as a
separate quoted argument; ensure the unique symbols mentioned (modify_dockerfile
and the sed invocation) are updated so the shell never sees $@ inside the sed
script.
There was a problem hiding this comment.
Not sure why changing this - need to be careful not to break existing comments. Once you take the main, you will see a few more was added.
There was a problem hiding this comment.
@nadav-t, thanks for the context — let me clarify why this matters.
The original code using / as delimiter was actually fine:
sed -i.bak "s/^\(.*${pattern}.*\)$/# \1/" "$temp_file"There's no $@ risk here because the delimiter is /.
The new code that changed to @ introduced a subtle bash expansion bug:
sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file"Inside a double-quoted string, bash sees \)$@ and expands $@ as the function's positional parameters — i.e., the dockerfile path and commands-file path get spliced into the middle of the sed expression. This causes sed to receive garbled arguments, silently breaking every COMMENT action.
The fix with mixed quoting keeps the @ delimiter (which is useful for patterns with / in them) while preventing the unintended $@ expansion:
sed -i.bak 's@^\(.*'"${pattern}"'.*\)$@# \1@' "$temp_file"Here $@ sits inside single quotes and is never touched by bash, while ${pattern} is still correctly expanded via the inner double-quoted segment.
🧠 Learnings used
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to config/*-modifications.txt : Dockerfile modification system uses format ACTION:PATTERN:REPLACEMENT in configuration files. Actions include: COMMENT, COMMENT_PAIR, COMMENT_BLOCK, REMOVE, REPLACE, ADD_AFTER, ADD_BEFORE
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to config/*-modifications.txt : Use COMMENT_BLOCK action to comment out entire multi-line RUN blocks (searches for pattern in comments or RUN commands)
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to config/*-modifications.txt : Use COMMENT_PAIR action for `ARG + RUN` version pairs - comment out matched line and immediately following non-empty line only when both match their respective patterns
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : Base images must contain only open-source tools, no proprietary Mend code
Learnt from: CR
Repo: mend/docker-base-images PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-17T00:07:50.932Z
Learning: Applies to repo-integrations/**/Dockerfile* : All source Dockerfiles must have `# END OF BASE IMAGE` comment to mark truncation point
| uses: actions/checkout@v3 | ||
| with: | ||
| ref: "main" | ||
| ref: ${{ github.ref }} |
| RELEASE=$1 | ||
| REGISTRY_PREFIX=$2 | ||
| COPY_VERSIONS_JSON=$3 | ||
| TAG_SUFFIX=$4 # Optional: "-arm64" or empty for AMD64 |
There was a problem hiding this comment.
probably need to update based on main
| if [ -z "$(docker images -q ${REGISTRY_PREFIX}/base-repo-scanner-sast:${RELEASE} 2> /dev/null)" ]; then | ||
| echo "❌ ${REGISTRY_PREFIX}/base-repo-scanner-sast:${RELEASE} was not built successfully" | ||
| if [ -z "$(docker images -q ${REGISTRY_PREFIX}/base-repo-scanner-sast:${RELEASE}${TAG_SUFFIX} 2> /dev/null)" ]; then | ||
| echo "❌ ${REGISTRY_PREFIX}/base-repo-scanner-sast:${RELEASE}${TAG_SUFFIX} was not built successfully" |
There was a problem hiding this comment.
I think it might be already part of the release tag make sure to take main, if not then looks fine
| exit 1 | ||
| fi | ||
|
|
||
| apply_dockerfile_modifications "repo-integrations/scanner/Dockerfile.full" "scanner" |
There was a problem hiding this comment.
I think it might be wrong - both files using scanner
There was a problem hiding this comment.
I see you changed the txt files . not sure why, it would break updates for both type of files.
Anyway the arm should be supported for both scanner files so I'm not sure what you gain, probably it's exactly the same changes.
| # Comment out lines matching pattern - simple single line | ||
| sed -i.bak "s/^\(.*${pattern}.*\)$/# \1/" "$temp_file" | ||
| # Use @ as delimiter to avoid conflicts with / in paths and | in pipes | ||
| sed -i.bak "s@^\(.*${pattern}.*\)$@# \1@" "$temp_file" |
There was a problem hiding this comment.
Not sure why changing this - need to be careful not to break existing comments. Once you take the main, you will see a few more was added.
| "REPLACE") | ||
| # Replace entire line matching pattern | ||
| sed -i.bak "s/.*${pattern}.*/${replacement}/" "$temp_file" | ||
| # Use @ as delimiter to avoid conflicts with / in paths and | in pipes |
3 participants
Summary by CodeRabbit
New Features
Chores