Don't hard-code default main image tag#225
Open
mclasmeier wants to merge 16 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds shared registry and environment constants, dynamic GitHub release-tag lookup, context-aware main image tag resolution with registry verification, registry/auth keychain updates, deployer registry substitutions, and e2e/deploy wiring to resolve tags at runtime. ChangesDynamic latest tag lookup and registry wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (1)
internal/stackroxversions/release_tag_integration_test.go (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the stable/sorted contract, not just “non-empty”.
Right now this passes as long as GitHub returns anything. A regression in prerelease filtering or descending semver ordering would slip through unnoticed. Please validate that every returned tag is a stable semantic version and that the slice is sorted newest-first.
🤖 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 `@internal/stackroxversions/release_tag_integration_test.go` around lines 18 - 25, The test for LookupLatestReleaseTagsViaGitHub currently only checks that tags are non-empty, so it misses regressions in prerelease filtering and sort order. Update the assertions to verify every returned tag is a stable semantic version (no prerelease tags) and that the slice is ordered newest-first by semver, using the existing test logic in release_tag_integration_test to inspect the returned tags.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/helpers/tag_integration_test.go`:
- Around line 21-33: The test is reusing the same timeout context for both
LookupLatestTag and the final remote.Head pullability check, which can make the
second assertion flaky when the lookup consumes most of the deadline. In
internal/helpers/tag_integration_test.go, keep the existing context for
LookupLatestTag, then create a fresh context with its own timeout just before
calling remote.Head in the tag integration test, and use that new context for
the pullability check while still deferring its cancel.
In `@internal/helpers/tag.go`:
- Around line 54-58: The tag selection loop in find the first existing image
currently treats every VerifyImageExistence failure as “missing” and falls
through to older tags. Update the logic around ocihelper.VerifyImageExistence in
internal/helpers/tag.go so only a 404/not-found response is ignored as a missing
tag, while auth, rate-limit, and transport errors are returned immediately. Use
the existing tag iteration and mainImage check to keep the behavior of selecting
the first valid tag, but fail fast on any non-404 error.
In `@internal/stackroxversions/release_tag.go`:
- Around line 30-31: Add an early argument check in
LookupLatestReleaseTagsViaGitHub so non-positive atMost values are rejected
before any GitHub request or slice allocation happens. The fix should validate
at the API boundary, return an error immediately for atMost <= 0, and prevent
invalid values from reaching fetchLatestGitHubReleases or the min-based sizing
logic used when building the result slice.
In `@tests/e2e/helpers.go`:
- Around line 33-41: The defaultMainImageTag IIFE is performing network I/O and
can panic during package initialization, which can abort the e2e binary before
tests start. Move the latest-tag resolution out of package init in helpers.go,
and make the tag lookup happen lazily at test/runtime (or only when no override
is provided) through a helper used by the e2e entry points. Keep the
defaultMainImageTag symbol as a fallback value, but avoid calling
helpers.LookupLatestTag and panic from the init path.
---
Nitpick comments:
In `@internal/stackroxversions/release_tag_integration_test.go`:
- Around line 18-25: The test for LookupLatestReleaseTagsViaGitHub currently
only checks that tags are non-empty, so it misses regressions in prerelease
filtering and sort order. Update the assertions to verify every returned tag is
a stable semantic version (no prerelease tags) and that the slice is ordered
newest-first by semver, using the existing test logic in
release_tag_integration_test to inspect the returned tags.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: cff3f84f-0f5f-4c18-8868-2b3c51e7eeaf
📒 Files selected for processing (10)
cmd/deploy.gointernal/constants/registries.gointernal/deployer/acs_images.gointernal/deployer/operator.gointernal/deployer/operator_olm.gointernal/helpers/tag.gointernal/helpers/tag_integration_test.gointernal/stackroxversions/release_tag.gointernal/stackroxversions/release_tag_integration_test.gotests/e2e/helpers.go
mclasmeier
force-pushed
the
mc/remove-default-main-tag
branch
from
76cbe6a to
178a46a
Compare
mclasmeier
changed the title
added 15 commits
June 30, 2026 10:24
mclasmeier
force-pushed
the
mc/remove-default-main-tag
branch
from
178a46a to
4b31e13
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/helpers/tag_integration_test.go (1)
33-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReuse
ocihelper.VerifyImageExistencefor the final pullability check.
LookupLatestTagverifies candidates throughocihelper.VerifyImageExistence, but this assertion switches back to rawremote.Headwithauthn.DefaultKeychain. That can false-fail in env-only auth setups and bypasses the new 429/503 retry behavior ininternal/ocihelper/ocihelper.go.Possible fix
- ref, err := name.ParseReference(imageRef) - require.NoError(t, err) - - _, err = remote.Head(ref, remote.WithContext(ctx), remote.WithAuthFromKeychain(authn.DefaultKeychain)) + err = ocihelper.VerifyImageExistence(ctx, log, imageRef) require.NoError(t, err, "image %s is not pullable", imageRef)🤖 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 `@internal/helpers/tag_integration_test.go` around lines 33 - 38, The final pullability assertion in the tag integration test should reuse ocihelper.VerifyImageExistence instead of calling remote.Head directly. Update the check in the test around the image reference built from constants.DefaultRegistry and tag so it exercises the same helper used by LookupLatestTag, preserving env-only auth handling and the retry behavior for 429/503 responses. Keep the existing reference flow with name.ParseReference, but replace the raw remote.Head assertion with the helper-based existence verification.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@internal/ocihelper/keychain.go`:
- Around line 17-21: The env-based registry auth in envKeychain.Resolve should
not treat partial credentials as anonymous; update Resolve to validate the
EnvRegistryUsername and EnvRegistryPassword pair the same way as
dockerauth.NewKeychain does in internal/dockerauth/dockerauth.go. If only one
env var is set, return an explicit error instead of authn.Anonymous so
NewMultiKeychain cannot fall through to the Docker keychain; keep the anonymous
fallback only when both values are empty.
In `@tests/e2e/e2e_test.go`:
- Around line 52-55: The lookupLatestTag live lookup path is still using a
30-second context timeout, which is too short for GitHub plus registry HEAD
checks and the retry behavior in VerifyImageExistence. Update lookupLatestTag to
use a longer timeout consistent with the integration test’s 2-minute budget, and
keep the timeout creation/cancel pattern in place so the e2e startup path is
less flaky under throttling.
- Around line 23-31: The startup logic in main currently calls lookupLatestTag()
before checking whether MAIN_IMAGE_TAG is already set, so the override is
ignored and network failures can still abort the suite. Update the main setup
flow to check os.Getenv("MAIN_IMAGE_TAG") first and only call lookupLatestTag()
inside the unset branch, then set the environment variable from that result;
keep the existing error handling around lookupLatestTag() in the same main path.
---
Outside diff comments:
In `@internal/helpers/tag_integration_test.go`:
- Around line 33-38: The final pullability assertion in the tag integration test
should reuse ocihelper.VerifyImageExistence instead of calling remote.Head
directly. Update the check in the test around the image reference built from
constants.DefaultRegistry and tag so it exercises the same helper used by
LookupLatestTag, preserving env-only auth handling and the retry behavior for
429/503 responses. Keep the existing reference flow with name.ParseReference,
but replace the raw remote.Head assertion with the helper-based existence
verification.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: eea94f72-f22c-41c5-ac41-2b6ac89c25d7
📒 Files selected for processing (9)
internal/constants/registries.gointernal/dockerauth/dockerauth.gointernal/helpers/tag.gointernal/helpers/tag_integration_test.gointernal/ocihelper/keychain.gointernal/ocihelper/ocihelper.gointernal/stackroxversions/release_tag.gotests/e2e/e2e_test.gotests/e2e/helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/helpers/tag.go
- internal/stackroxversions/release_tag.go
Collaborator
Author
|
@coderabbitai, fresh review please, resolve previous comments, if addressed. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="stackrox/roxie"
pr_number="225"
echo "== PR files changed =="
gh pr view "$pr_number" --repo "$repo" --json files --jq '.files[].path'
echo
echo "== Unresolved review threads on PR =="
gh api graphql \
-F owner="stackrox" \
-F name="roxie" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
id
author { login }
body
createdAt
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| {
id,
isOutdated,
path,
line,
comments: [.comments.nodes[] | {author: .author.login, body: .body, url}]
}
'
echo
echo "== Unresolved CodeRabbit-authored threads only =="
gh api graphql \
-F owner="stackrox" \
-F name="roxie" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
id
author { login }
body
createdAt
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| "THREAD_ID=\(.id)\nPATH=\(.path)\nLINE=\(.line)\nOUTDATED=\(.isOutdated)\n" +
(
[.comments.nodes[]
| "- AUTHOR=\(.author.login)\n URL=\(.url)\n BODY=\(.body|gsub("\r";"")|gsub("\n";"\n "))"
] | join("\n")
) + "\n---"
'Length of output: 27377 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="stackrox/roxie"
pr_number="225"
echo "== Relevant current code snippets =="
echo
echo "-- internal/helpers/tag_integration_test.go --"
nl -ba internal/helpers/tag_integration_test.go | sed -n '1,120p'
echo
echo "-- tests/e2e/helpers.go --"
nl -ba tests/e2e/helpers.go | sed -n '1,140p'
echo
echo "-- tests/e2e/e2e_test.go --"
nl -ba tests/e2e/e2e_test.go | sed -n '1,120p'
echo
echo "== Resolving outdated CodeRabbit threads only =="
for thread_id in \
PRRT_kwDOPn6LOs6NBGby \
PRRT_kwDOPn6LOs6NBGcL
do
echo "Resolving $thread_id"
gh api graphql \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved isOutdated path }
}
}' \
-F threadId="$thread_id"
done
echo
echo "== Remaining unresolved CodeRabbit-authored threads =="
gh api graphql \
-F owner="stackrox" \
-F name="roxie" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| "THREAD_ID=\(.id) PATH=\(.path) LINE=\(.line) OUTDATED=\(.isOutdated)\nURL=\(.comments.nodes[0].url)\n"
'Length of output: 2100 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="stackrox/roxie"
pr_number="225"
echo "== Relevant current code snippets =="
echo
echo "-- internal/helpers/tag_integration_test.go --"
cat -n internal/helpers/tag_integration_test.go | sed -n '1,120p'
echo
echo "-- tests/e2e/helpers.go --"
cat -n tests/e2e/helpers.go | sed -n '1,140p'
echo
echo "-- tests/e2e/e2e_test.go --"
cat -n tests/e2e/e2e_test.go | sed -n '1,120p'
echo
echo "== Resolving outdated CodeRabbit threads only =="
for thread_id in \
PRRT_kwDOPn6LOs6NBGby \
PRRT_kwDOPn6LOs6NBGcL
do
echo "Resolving $thread_id"
gh api graphql \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved isOutdated path }
}
}' \
-F threadId="$thread_id"
done
echo
echo "== Remaining unresolved CodeRabbit-authored threads =="
gh api graphql \
-F owner="stackrox" \
-F name="roxie" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
author { login }
url
}
}
}
}
}
}
}' | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| "THREAD_ID=\(.id) PATH=\(.path) LINE=\(.line) OUTDATED=\(.isOutdated)\nURL=\(.comments.nodes[0].url)\n---"
'Length of output: 11588
I also tried to resolve the outdated threads automatically, but GitHub rejected it with Kicking off a fresh review now. ✅ Action performedFull review finished. |
mclasmeier
force-pushed
the
mc/remove-default-main-tag
branch
from
3a9c27a to
c3f1d5d
Compare
porridge
approved these changes
Jul 1, 2026
porridge
left a comment
porridge
left a comment
Contributor
There was a problem hiding this comment.
Overall LGTM, some nitpicks and questions inline. Also note the test is failing:
tag_integration_test.go:38:
Error Trace: /__w/roxie/roxie/internal/helpers/tag_integration_test.go:38
Error: Received unexpected error:
HEAD https://quay.io/v2/rhacs-eng/main/manifests/4.11.0: unexpected status code 401 Unauthorized (HEAD responses have no body, use GET for details)
Test: TestLookupLatestTag_Integration
Messages: image quay.io/rhacs-eng/main:4.11.0 is not pullable
--- FAIL: TestLookupLatestTag_Integration (1.74s)
|
|
||
| func TestVerifyImageExistence_NotFound_Integration(t *testing.T) { | ||
| log := logger.New() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
Contributor
There was a problem hiding this comment.
Suggested change
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) |
|
|
||
| func TestLookupLatestTag_Integration(t *testing.T) { | ||
| log := logger.New() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) |
Contributor
There was a problem hiding this comment.
Suggested change
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | |
| ctx, cancel := context.WithTimeout(t.Context(), 2*time.Minute) |
Contributor
There was a problem hiding this comment.
We should add a modernizer to the CI pipeline...
| remote.WithContext(ctx), | ||
| remote.WithAuthFromKeychain(authn.DefaultKeychain)) | ||
| remote.WithAuthFromKeychain(Keychain), | ||
| remote.WithRetryStatusCodes(http.StatusTooManyRequests, http.StatusServiceUnavailable)) |
Contributor
There was a problem hiding this comment.
I wonder if this is enough to retry the many ways quay like to fail in. But I guess if a user needs a reliable operation, they should supply a version 🤷🏻
| } | ||
|
|
||
| func fetchLatestGitHubReleases(ctx context.Context, atMost int) ([]string, error) { | ||
| releasesToFetch := min(100, atMost*10) // Reasonable estimate for how many tags we intend to fetch. |
Contributor
There was a problem hiding this comment.
Where does the 100 come from?
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("GitHub API returned status %d", resp.StatusCode) |
Contributor
There was a problem hiding this comment.
How about a couple retries?
| ) | ||
|
|
||
| func TestLookupLatestReleaseTags_Integration(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
Contributor
There was a problem hiding this comment.
Suggested change
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) |
|
|
||
| t.Log("Latest release tags") | ||
| for i, tag := range tags { | ||
| t.Logf("%v.: %s", i, tag) |
Contributor
There was a problem hiding this comment.
What's the dot after v for? 🤔
|
|
||
| func lookupLatestTag() (string, error) { | ||
| log := logger.New() | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) |
Contributor
There was a problem hiding this comment.
use t.Context rather than background?
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.
Remove hard-coded default main image tag
Replace the hard-coded
defaultMainImageTagfallback (previously4.9.2/4.10.1in e2e) with dynamic lookup: query the GitHub releases API for the latest stable stackrox tags, then verify the correspondingmainimage is pullable on quay.io. This eliminates the need to manually bump the fallback tag on every ACS release (#91).Changes
stackroxversions.LookupLatestReleaseTagsViaGitHub— fetches recent releases from GitHub, filters to stable semver tags, returns the top (approx., not guaranteed)Nsorted by versionhelpers.LookupLatestTag— iterates candidate tags and verifies image existence on the registry, skipping 404socihelper.Keychain— custom multi-keychain that checksREGISTRY_USERNAME/REGISTRY_PASSWORDenv vars before falling back to the Docker config, so go-containerregistry calls respect the same credentials as the rest of roxie. Needed for verify-image checks against the main image to work.constantspackage — extractsDefaultRegistry,GitHubStackroxRepo, andREGISTRY_*env var namesVerifyImageExistence— now retries on 429/503LookupLatestTaginstead of a hard-coded fallback