ROX-33123: Bump apollo-ci image to 0.5.4#19778
Closed
mclasmeier wants to merge 7 commits intomasterfrom
Closed
Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
Author
|
/test gke-run-scanner-v4-install-tests |
Contributor
Author
|
/test gke-scanner-v4-install-tests |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In tests/e2e/run-scanner-v4-install.bats a large set of Scanner v4 install/upgrade/operator tests has been removed and replaced with a trivial
@test "Invokie roxie"block; please confirm this file change is intentional, as it looks like an accidental overwrite rather than a minimal image-bump adjustment. - The new test name
@test "Invokie roxie"contains a typo and the test body just callsroxie versionandroxie envwithout any assertions; if the test is meant to stay, consider renaming it and adding checks on command success or expected output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In tests/e2e/run-scanner-v4-install.bats a large set of Scanner v4 install/upgrade/operator tests has been removed and replaced with a trivial `@test "Invokie roxie"` block; please confirm this file change is intentional, as it looks like an accidental overwrite rather than a minimal image-bump adjustment.
- The new test name `@test "Invokie roxie"` contains a typo and the test body just calls `roxie version` and `roxie env` without any assertions; if the test is meant to stay, consider renaming it and adding checks on command success or expected output.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughWalkthroughThis pull request updates CI container image versions from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 `@tests/e2e/run-scanner-v4-install.bats`:
- Around line 445-448: Rename the test case label "Invokie roxie" to the correct
spelling "Invoke roxie" in the `@test` block (the block that runs the commands
`roxie version` and `roxie env`), and if this change was intended only for
temporary CI validation (replacing multiple Scanner v4 install/upgrade e2e
tests), restore the previously removed tests before merging to master so you
don’t lose the full installation/upgrade coverage.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c2ce8e6f-99fa-4382-bcbf-482430af21c2
📒 Files selected for processing (17)
.devcontainer/devcontainer.json.github/workflows/batch-load-test-metrics.yml.github/workflows/build.yaml.github/workflows/check-crd-compatibility.yaml.github/workflows/ci-failures-report.yml.github/workflows/fixxxer.yaml.github/workflows/scanner-build.yaml.github/workflows/scanner-db-integration-tests.yaml.github/workflows/scanner-versioned-definitions-update.yaml.github/workflows/unit-tests.yaml.openshift-ci/Dockerfile.build_root.openshift-ci/dev-requirements.txtBUILD_IMAGE_VERSIONoperator/bundle_helpers/requirements.txtscale/signatures/deploy.yamltests/e2e/run-e2e-tests.shtests/e2e/run-scanner-v4-install.bats
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughContainer image tags are systematically updated from version 0.5.3 to 0.5.4 across GitHub Actions workflows, build configurations, Kubernetes manifests, and shell scripts. A large set of Scanner v4 e2e installation tests are replaced with a single minimal test. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Contributor
|
Images are ready for the commit at cfc5f4e. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19778 +/- ##
=======================================
Coverage 49.59% 49.59%
=======================================
Files 2761 2761
Lines 208143 208143
=======================================
+ Hits 103226 103229 +3
+ Misses 97252 97250 -2
+ Partials 7665 7664 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cfc5f4e to
6c1373e
Compare
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/bump-apollo-ci-openshift-release/SKILL.md:
- Around line 135-144: The fenced code block containing the success message (the
triple-backtick block starting with "✅ Successfully bumped apollo-ci to version
NEW_VERSION!") is missing a language identifier; add a language tag (e.g.,
"text" or "md") after the opening ``` so the block becomes ```text to resolve
MD040 and normalize rendering/tooling.
- Around line 46-50: The current clean-tree check uses "git -C RELEASE_REPO_PATH
diff-index --quiet HEAD --" which ignores untracked files; change the check to
detect any working-tree changes including untracked files by running "git -C
RELEASE_REPO_PATH status --porcelain" (or using "git -C RELEASE_REPO_PATH
ls-files --others --exclude-standard") and treat any non-empty output as dirty:
if the porcelain/ls-files output is non-empty, echo the status (use the existing
git -C RELEASE_REPO_PATH status --short for display) and exit 1; update the
logic around RELEASE_REPO_PATH accordingly so untracked files fail the
clean-tree check.
- Around line 81-83: Replace the non-portable sed invocation using "sed -i ''"
in the xargs/sed commands with a portable approach: use "sed -i.bak" so the
substitution line (the one matching "s/tag:
(stackrox-test|scanner-test|stackrox-ui-test)-[0-9]+\.[0-9]+\.[0-9]+$/tag:
\1-NEW_VERSION/") works on both GNU and BSD sed, and then add a cleanup step to
remove the generated .bak files (the same change should be applied to the other
sed usage referenced around lines 90-92); update the SKILL.md script sections
containing the xargs/sed commands to create backups with -i.bak and delete those
.bak files afterwards.
In @.claude/skills/bump-apollo-ci/SKILL.md:
- Around line 112-123: The markdown has fenced code blocks missing language
identifiers (MD040); update the block that starts with "✅ Changes committed to
branch: BRANCH_NAME" to use a language tag like ```text and ensure the shell
snippet containing the git commands (the block with git grep "OLD_VERSION" and
git status) is tagged as ```bash; search for the two blocks (one beginning with
the branch commit message and the other with the git commands) and add the
appropriate language identifiers to each so they validate as MD040-compliant.
- Around line 47-51: The current clean check uses "git diff-index --quiet HEAD
--", which ignores untracked files; update the script to detect untracked files
too by replacing that condition with a check that fails when "git status
--porcelain --untracked-files=all" produces any output (i.e. run git status
--porcelain --untracked-files=all and test for non-empty output), then keep the
existing echo and git status --short reporting and exit 1 on dirty repo.
- Around line 73-75: The sed replacement only targets strings like
"apollo-ci:...-OLD_VERSION" but the document also contains standalone
"stackrox-build-OLD_VERSION" mentions; update the replacement to cover both
forms by changing the regex in the sed invocation (the command that currently
contains
apollo-ci:\(stackrox-test\|scanner-test\|stackrox-ui-test\|stackrox-build\)-OLD_VERSION)
to also match optional "apollo-ci:" or a bare "stackrox-build-OLD_VERSION" (for
example make the left side allow either apollo-ci:GROUP-OLD_VERSION or
stackrox-build-OLD_VERSION), so references to stackrox-build-OLD_VERSION are
replaced as intended.
- Around line 73-75: The sed usage in the git ls-files pipeline uses
BSD-specific in-place syntax `sed -i ''` which fails on GNU sed; update the
command that pipes into `xargs sed -i ''
's/apollo-ci:\(stackrox-test\|scanner-test\|stackrox-ui-test\|stackrox-build\)-OLD_VERSION/apollo-ci:\1-NEW_VERSION/g'`
to a portable approach — either use `sed -i.bak` for cross-platform in-place
edits and add a cleanup step to remove `*.bak` backups, or implement a
POSIX-safe rewrite using `sed 's/.../.../' file > file.new && mv file.new file`
(i.e., change the pipeline to one of these alternatives and ensure removal of
backup files).
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 80ad7c15-88bf-4a9b-84ab-a84efaa5d0be
📒 Files selected for processing (4)
.claude/commands/bump-apollo-ci-openshift-release.md.claude/commands/bump-apollo-ci.md.claude/skills/bump-apollo-ci-openshift-release/SKILL.md.claude/skills/bump-apollo-ci/SKILL.md
✅ Files skipped from review due to trivial changes (2)
- .claude/commands/bump-apollo-ci-openshift-release.md
- .claude/commands/bump-apollo-ci.md
Comment on lines
+81
to
+83
| xargs -I {} sed -i '' -E \ | ||
| 's/tag: (stackrox-test|scanner-test|stackrox-ui-test)-[0-9]+\.[0-9]+\.[0-9]+$/tag: \1-NEW_VERSION/' \ | ||
| "RELEASE_REPO_PATH/{}" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is sed -i '' supported on GNU sed, and what is the recommended cross-platform in-place editing pattern for bash scripts?
💡 Result:
No, sed -i '' is not supported on GNU sed for no-backup in-place editing. GNU sed's -i option takes an optional [SUFFIX] immediately attached (e.g., -i.bak for backup with .bak extension, or -i alone for no backup). Providing a separate empty argument '' after -i (as in sed -i '') causes GNU sed to treat '' as the input file name, leading to errors like "sed: can't read '': No such file or directory". The recommended cross-platform pattern for bash scripts targeting GNU sed (Linux) and BSD sed (macOS/FreeBSD) is to detect the sed version and use appropriate flags: #!/bin/bash if sed --help 2>&1 | grep -q GNU; then # GNU sed: use -i alone for no backup sed -i 's/old/new/g' file.txt else # BSD sed: use -i '' for no backup sed -i '' 's/old/new/g' file.txt fi Or, more portably (POSIX-compliant, no -i), use a temporary file: sed 's/old/new/g' file.txt > file.txt.tmp && mv file.txt.tmp file.txt This manual approach avoids implementation differences entirely and is safer (preserves permissions, handles symlinks better).
Citations:
- 1: https://www.gnu.org/software/sed/manual/html_node/Command_002dLine-Options.html
- 2: https://www.gnu.org/software/sed/manual/sed.html
- 3: https://stackoverflow.com/questions/21242932/sed-i-may-not-be-used-with-stdin-on-mac-os-x
- 4: https://unix.stackexchange.com/questions/13711/differences-between-sed-on-mac-osx-and-other-standard-sed
- 5: https://unix.stackexchange.com/questions/92895/how-can-i-achieve-portability-with-sed-i-in-place-editing
- 6: https://stackoverflow.com/questions/5694228/sed-in-place-flag-that-works-both-on-mac-bsd-and-linux
- 7: https://stackoverflow.com/questions/12696125/sed-edit-file-in-place
Use portable sed -i.bak syntax for cross-platform compatibility
Lines 81 and 90 use sed -i '', which fails on GNU sed (Linux) as it treats the empty string as a filename, producing "sed: can't read '': No such file or directory". Use sed -i.bak instead—supported by both GNU sed and BSD sed—then clean up the backup files.
Portable approach
-git -C RELEASE_REPO_PATH ls-files "$PATTERN" | \
- xargs -I {} sed -i '' -E \
- 's/tag: (stackrox-test|scanner-test|stackrox-ui-test)-[0-9]+\.[0-9]+\.[0-9]+$/tag: \1-NEW_VERSION/' \
- "RELEASE_REPO_PATH/{}"
+git -C RELEASE_REPO_PATH ls-files "$PATTERN" | \
+ xargs -I {} sed -i.bak -E \
+ 's/tag: (stackrox-test|scanner-test|stackrox-ui-test)-[0-9]+\.[0-9]+\.[0-9]+$/tag: \1-NEW_VERSION/' \
+ "RELEASE_REPO_PATH/{}" && \
+git -C RELEASE_REPO_PATH ls-files "$PATTERN" | xargs -I {} rm -f "RELEASE_REPO_PATH/{}.bak"Also applies to: 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/bump-apollo-ci-openshift-release/SKILL.md around lines 81 -
83, Replace the non-portable sed invocation using "sed -i ''" in the xargs/sed
commands with a portable approach: use "sed -i.bak" so the substitution line
(the one matching "s/tag:
(stackrox-test|scanner-test|stackrox-ui-test)-[0-9]+\.[0-9]+\.[0-9]+$/tag:
\1-NEW_VERSION/") works on both GNU and BSD sed, and then add a cleanup step to
remove the generated .bak files (the same change should be applied to the other
sed usage referenced around lines 90-92); update the SKILL.md script sections
containing the xargs/sed commands to create backups with -i.bak and delete those
.bak files afterwards.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The shell snippets in the new bump-apollo-ci skills use
sed -i '' ..., which is macOS/BSD-specific and will fail on typical GNU/Linux developer/CI environments; consider using a portable pattern (e.g.,sed -i.bakwith a follow-uprmof the backup) or explicitly documenting the required platform. - In
.claude/skills/bump-apollo-ci-openshift-release/SKILL.md, commands like"RELEASE_REPO_PATH/{}"appear to use the literal string instead of the$RELEASE_REPO_PATHvariable, and the same applies to somegit -C RELEASE_REPO_PATHexamples—these should be updated to interpolate the variable correctly so the instructions work when copied verbatim. - The bump-apollo-ci skill hardcodes
origin/masteras the base branch when creating the bump branch; if this repo’s default branch is (or ever becomes)main, consider either referencing the default branch dynamically or at least calling this assumption out in the instructions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The shell snippets in the new bump-apollo-ci skills use `sed -i '' ...`, which is macOS/BSD-specific and will fail on typical GNU/Linux developer/CI environments; consider using a portable pattern (e.g., `sed -i.bak` with a follow-up `rm` of the backup) or explicitly documenting the required platform.
- In `.claude/skills/bump-apollo-ci-openshift-release/SKILL.md`, commands like `"RELEASE_REPO_PATH/{}"` appear to use the literal string instead of the `$RELEASE_REPO_PATH` variable, and the same applies to some `git -C RELEASE_REPO_PATH` examples—these should be updated to interpolate the variable correctly so the instructions work when copied verbatim.
- The bump-apollo-ci skill hardcodes `origin/master` as the base branch when creating the bump branch; if this repo’s default branch is (or ever becomes) `main`, consider either referencing the default branch dynamically or at least calling this assumption out in the instructions.
## Individual Comments
### Comment 1
<location path=".claude/skills/bump-apollo-ci/SKILL.md" line_range="35-36" />
<code_context>
+## Procedure
+
+### 1. Get versions and scope
+Ask user if not provided:
+- OLD_VERSION (e.g., "0.5.4")
+- NEW_VERSION (e.g., "0.5.5")
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding "the" for smoother grammar in this sentence.
You could change this line to: `Ask the user if not provided:` to fix the grammar while keeping the meaning the same.
```suggestion
### 1. Get versions and scope
Ask the user if not provided:
```
</issue_to_address>
### Comment 2
<location path=".claude/commands/bump-apollo-ci.md" line_range="13" />
<code_context>
+Ask the user:
+- OLD_VERSION (current version to replace, e.g., "0.5.4")
+- NEW_VERSION (target version, e.g., "0.5.5")
+- Also bump in openshift/release repository? If yes, use `bump-apollo-ci-openshift-release` skill after completing stackrox bump
+
+Then systematically update ALL occurrences following the skill documentation:
</code_context>
<issue_to_address>
**nitpick (typo):** Minor wording tweak: add an article before "stackrox bump" for clarity.
Change the phrase to “after completing the stackrox bump” (or “the StackRox bump”) so the sentence flows more naturally.
```suggestion
- Also bump in openshift/release repository? If yes, use `bump-apollo-ci-openshift-release` skill after completing the stackrox bump
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…/apollo-ci-bump-roxie
|
@mclasmeier: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a preparation for https://redhat.atlassian.net/browse/ROX-33123.
This PR does not only include the apollo-ci bumps -- it also includes new claude skills for bumping apollo-ci versions in
stackrox/stackroxand inopenshift/release.User-facing documentation
Not needed.
Testing and quality
Automated testing
Nothing added.
How I validated my change
Just CI.