fix: host-runner compatibility for tests and scripts#19712
Merged
Conversation
Fix tests and scripts to work on ubuntu-latest host runners (no container) in addition to CI container environments. - rate_limited_logger.go: add /home/runner/work/ path prefix (container used /__w/) - download_zip_test.go: accept ErrPermission for /root/ paths (non-root runner returns permission denied, not not-found) - detection_test.go: delete circular test - lib.sh: escape & in bash 5.2+ replacements (verified 4.4-5.3) - helpers.bash: add yq_multidoc helper (yq 4.x adds --- separators) - roxctl-netpol-generate-*.bats: use yq_multidoc All changes are backward-compatible with container environments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
yq_multidochelper blindly strips any line equal to---, which could hide legitimate content in future YAML outputs; consider making the filtering more targeted (e.g., only when multi-doc mode is expected or using yq flags/options to control separators instead of post-processing). - The hard-coded
githubHostPrefix = "/home/runner/work/stackrox/stackrox/"inrate_limited_logger.gois tightly coupled to the current GitHub runner layout; it may be more robust to derive the prefix fromGITHUB_WORKSPACEor to trim dynamically based on the repo path instead of specific absolute paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `yq_multidoc` helper blindly strips any line equal to `---`, which could hide legitimate content in future YAML outputs; consider making the filtering more targeted (e.g., only when multi-doc mode is expected or using yq flags/options to control separators instead of post-processing).
- The hard-coded `githubHostPrefix = "/home/runner/work/stackrox/stackrox/"` in `rate_limited_logger.go` is tightly coupled to the current GitHub runner layout; it may be more robust to derive the prefix from `GITHUB_WORKSPACE` or to trim dynamically based on the repo path instead of specific absolute paths.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughWalkthroughThe pull request removes a container detection test, updates test error handling, adds a path prefix to file path trimming logic, fixes XML-escaping in CI output generation, introduces a YAML processing helper function, and updates BATS tests to use the new helper for multi-document YAML validation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
|
Images are ready for the commit at 58ea483. 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 #19712 +/- ##
==========================================
- Coverage 49.59% 49.59% -0.01%
==========================================
Files 2756 2756
Lines 208036 208036
==========================================
- Hits 103183 103180 -3
- Misses 97192 97196 +4
+ Partials 7661 7660 -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:
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
getTrimmedFilePath, instead of hardcoding the/home/runner/work/stackrox/stackrox/host-runner prefix, consider deriving the workspace prefix from an environment variable (e.g.GITHUB_WORKSPACE) or making it configurable so the logic is robust to path changes and non-GHA environments. - The new
yq_multidochelper will causegrep -v '^---$'to exit non‑zero when the only output lines are---, which will fail the caller even thoughyqsucceeded; consider tolerating an empty filtered output (e.g.grep -v '^---$' <<< "$output" || true) or handling this case explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getTrimmedFilePath`, instead of hardcoding the `/home/runner/work/stackrox/stackrox/` host-runner prefix, consider deriving the workspace prefix from an environment variable (e.g. `GITHUB_WORKSPACE`) or making it configurable so the logic is robust to path changes and non-GHA environments.
- The new `yq_multidoc` helper will cause `grep -v '^---$'` to exit non‑zero when the only output lines are `---`, which will fail the caller even though `yq` succeeded; consider tolerating an empty filtered output (e.g. `grep -v '^---$' <<< "$output" || true`) or handling this case explicitly.
## Individual Comments
### Comment 1
<location path="tests/roxctl/bats-tests/helpers.bash" line_range="14-17" />
<code_context>
+# yq_multidoc runs yq and strips --- document separators from output.
+# yq 4.x adds separators between multi-doc results which shift assert_line indices.
+yq_multidoc() {
+ local output
+ output=$(yq "$@") || return $?
+ grep -v '^---$' <<< "$output"
+}
+
</code_context>
<issue_to_address>
**issue:** yq_multidoc may fail when yq outputs only '---' lines or no lines at all because grep -v will return a non‑zero exit code
This helper has an edge case: if `yq` outputs only `---` separators (or nothing), `grep -v '^---$'` exits with status 1, making it look like `yq` failed and causing spurious test failures. To avoid this, consider using `sed '/^---$/d'` instead, or appending `|| true` to the `grep` call while preserving the original `yq` exit code (e.g., capture `$?` right after `yq` and return that).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
grep -v returns exit code 1 when no lines match (e.g. yq output is only --- separators), which would look like a yq failure. sed always returns 0 regardless of whether lines were deleted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
getTrimmedFilePath, the list of hard-coded GitHub path prefixes is starting to grow; consider centralizing these host/container path mappings or deriving them from an environment variable to avoid further proliferation of magic strings tied to specific CI layouts. - The updated assertion in
TestExtractZipToFolder_PreventPathTraversalusingassert.Truewith a compounderrors.Iscondition is a bit opaque; consider extracting this into a small helper (e.g.assertPathNotWritten(t, err)) or using a clearer assertion pattern to make the intent and allowed error types more obvious.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getTrimmedFilePath`, the list of hard-coded GitHub path prefixes is starting to grow; consider centralizing these host/container path mappings or deriving them from an environment variable to avoid further proliferation of magic strings tied to specific CI layouts.
- The updated assertion in `TestExtractZipToFolder_PreventPathTraversal` using `assert.True` with a compound `errors.Is` condition is a bit opaque; consider extracting this into a small helper (e.g. `assertPathNotWritten(t, err)`) or using a clearer assertion pattern to make the intent and allowed error types more obvious.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
davdhacs
added a commit
that referenced
this pull request
Apr 1, 2026
Run Go test jobs directly on ubuntu-latest instead of in the CI container image. This saves ~55s container init overhead per job and enables faster cache I/O (no overlay filesystem). Changes per job: - Remove container: block - Add actions/setup-go with GOTOOLCHAIN=auto override - Replace su postgres with docker run postgres - Add pg_isready retry loop (docker postgres needs warmup time) - go-bench: skip on PRs (if: github.event_name == 'push') Depends on #19712 (host-runner test fixes). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
davdhacs
added a commit
that referenced
this pull request
Apr 1, 2026
Run remaining unit-tests jobs directly on ubuntu-latest: - ui: remove container, add setup-node - ui-component: remove container, use cypress-io/github-action - local-roxctl-tests: remove container, add setup-go - shell-unit-tests: remove container, add bats-core/bats-action - openshift-ci-unit-tests: remove container Depends on #19712 (host-runner test fixes). Split from #19678. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 tasks
5 tasks
janisz
approved these changes
Apr 2, 2026
Contributor
Author
|
/test ? |
|
@davdhacs: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions 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. |
Contributor
🚀 Build Images ReadyImages are ready for commit 64ecebe. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-544-g64ecebe08f |
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
3 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
Fix tests and scripts to work on ubuntu-latest host runners (no container) in addition to CI container environments. All changes are backward-compatible — CI still passes in containers.
This is a prerequisite for removing containers from unit-test jobs (#19678).
Changes
rate_limited_logger.go/home/runner/work/not recognized (container used/__w/)download_zip_test.goos.Stat("/root/evil.txt")returnsErrPermissionon non-root (container ran as root)ErrNotExistorErrPermissiondetection_test.golib.sh&in${var//pat/repl}as matched pattern (container had bash 5.1)\&(works on all versions 4.4-5.3, verified via docker)helpers.bash---separators in multi-doc output (container had yq 4.16)yq_multidochelper that strips separators*-netpol-generate-*.batsyq_multidocVerification
These changes run in the existing CI container workflow (no workflow changes in this PR). If all tests pass, the fixes are proven backward-compatible.
Testing and quality
Automated testing
How I validated my change
CI runs:
A: in the existing rox-ci-image container, proving backward compatibility.
B: without a container, passing on #19678
Partially generated by AI.