perf(ci): remove container from ui, shell, roxctl, openshift-ci jobs#19745
Draft
perf(ci): remove container from ui, shell, roxctl, openshift-ci jobs#19745
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>
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>
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>
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- By replacing
make ui-component-testswithcypress-io/github-action@v7, make sure any project-specific setup thatmakeused to perform (builds, env vars, custom reporters, config flags) is not lost; if it did more than just run Cypress, consider extracting that logic into a reusable script invoked by the Cypress action. - For long-lived CI workflow dependencies like
cypress-io/github-action@v7andbats-core/bats-action@v4.0.0, consider pinning to a specific commit SHA instead of a moving tag to keep runs reproducible and avoid unexpected behavior changes from upstream updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By replacing `make ui-component-tests` with `cypress-io/github-action@v7`, make sure any project-specific setup that `make` used to perform (builds, env vars, custom reporters, config flags) is not lost; if it did more than just run Cypress, consider extracting that logic into a reusable script invoked by the Cypress action.
- For long-lived CI workflow dependencies like `cypress-io/github-action@v7` and `bats-core/bats-action@v4.0.0`, consider pinning to a specific commit SHA instead of a moving tag to keep runs reproducible and avoid unexpected behavior changes from upstream updates.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at eb7f80b. 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 #19745 +/- ##
=======================================
Coverage 49.59% 49.60%
=======================================
Files 2760 2760
Lines 208144 208144
=======================================
+ Hits 103239 103244 +5
+ Misses 97239 97237 -2
+ Partials 7666 7663 -3
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:
|
…tainer-other-tests
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughCI/CD workflows transition from Docker container execution to native setup actions for Node.js, Go, and Bash tooling. Test infrastructure updates include helper functions for multi-document YAML processing and assertion loosening for security testing. Production code updates add GitHub Actions workspace path trimming to logging and fix XML entity escaping in CI scripts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/unit-tests.yaml (2)
330-331: Prefer a job-levelenvforGOTOOLCHAIN.Lines 330-331 work, but they make the toolchain contract depend on step order. Declaring
GOTOOLCHAIN: autoonlocal-roxctl-testskeeps it visible next to the rest of the job configuration and automatically covers any future Go-related step inserted above this one.♻️ Suggested cleanup
local-roxctl-tests: runs-on: ubuntu-latest + env: + GOTOOLCHAIN: auto outputs: new-jiras: ${{ steps.junit2jira.outputs.new-jiras }} ... - - name: Override GOTOOLCHAIN - run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 330 - 331, Move the GOTOOLCHAIN assignment from a step that appends to $GITHUB_ENV into the job-level environment for the local-roxctl-tests job: remove the step that runs echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" and instead add GOTOOLCHAIN: auto under the env block for the local-roxctl-tests job so the variable is declared alongside the job configuration and applies to all steps consistently.
235-237: Pin the newly added action refs to immutable SHAs.Lines 235-237, 274-277, 283-288, 325-328, and Line 373 add more mutable
@v*refs. That keeps the host-runner migration simple, but it also means upstream retagging can change CI behavior without a repo change. Please pin the new actions to full commit SHAs before this lands.Also applies to: 274-277, 283-288, 325-328, 373-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 235 - 237, Replace mutable action refs (e.g., actions/setup-node@v6, actions/checkout@v4, actions/cache@v4, dorny/paths-filter@v2, etc.) with their immutable full commit SHAs in the workflow so upstream retags can't silently change CI behavior: locate each usage of those action refs (the occurrences added around the referenced lines) and update the ref from the short `@v`* tag to the corresponding full commit SHA from the action's GitHub repository, keeping the rest of the step config identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/unit-tests.yaml:
- Around line 330-331: Move the GOTOOLCHAIN assignment from a step that appends
to $GITHUB_ENV into the job-level environment for the local-roxctl-tests job:
remove the step that runs echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" and instead
add GOTOOLCHAIN: auto under the env block for the local-roxctl-tests job so the
variable is declared alongside the job configuration and applies to all steps
consistently.
- Around line 235-237: Replace mutable action refs (e.g., actions/setup-node@v6,
actions/checkout@v4, actions/cache@v4, dorny/paths-filter@v2, etc.) with their
immutable full commit SHAs in the workflow so upstream retags can't silently
change CI behavior: locate each usage of those action refs (the occurrences
added around the referenced lines) and update the ref from the short `@v`* tag to
the corresponding full commit SHA from the action's GitHub repository, keeping
the rest of the step config identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a17df3ea-3ad3-447c-8c85-3cf75f640dd2
📒 Files selected for processing (8)
.github/workflows/unit-tests.yamlpkg/containers/detection_test.gopkg/logging/rate_limited_logger.goroxctl/common/zipdownload/download_zip_test.goscripts/ci/lib.shtests/roxctl/bats-tests/helpers.bashtests/roxctl/bats-tests/local/roxctl-netpol-generate-development.batstests/roxctl/bats-tests/local/roxctl-netpol-generate-release.bats
💤 Files with no reviewable changes (1)
- pkg/containers/detection_test.go
- local-roxctl-tests: add bats-core/bats-action (bats not on ubuntu-latest) - openshift-ci-unit-tests: install pycodestyle (was in CI container) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bats-core/bats-action uses 4.0.0, not v4.0.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Remove CI container from non-Go-test jobs, running directly on
ubuntu-latest:setup-nodemake ui-component-testswithcypress-io/github-action@v7(caches Cypress binary automatically)setup-go+GOTOOLCHAIN=autobats-core/bats-action(bats not available on ubuntu-latest by default)Saves ~55s container init per job. Independent of Go test container removal (#19743).
Stacked on #19712 (host-runner test fixes).
Split from #19678.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Previously validated on #19678.
🤖 Generated with Claude Code