promoted-image-governor openshift mapping pullspec styles#5184
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWraps client Get errors with %w and replaces legacy fmt-based source pullspec construction by calling api.QuayImageReference using mappingConfig.SourceNamespace and the promoted tag. Tests updated to expect quay-proxy-style source pullspecs and a duplicated init() was removed. ChangesPullspec and tests updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/promoted-image-governor/main.go (1)
230-236: ⚡ Quick winAdd field-level docs for
SourcePullspecStyleaccepted values and default behavior.
OpenshiftMappingConfig.SourcePullspecStyleis exported but undocumented. A short field comment with allowed values will prevent config misuse.As per coding guidelines "**/*.go: Ensure Go documentation on functions, classes, and fields are written properly".
🤖 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 `@cmd/promoted-image-governor/main.go` around lines 230 - 236, Add a Go doc comment for the exported field SourcePullspecStyle on the OpenshiftMappingConfig struct describing the accepted string values (e.g., "legacy", "ocp", or whatever the code expects), the semantics of each value, and the default behavior when the field is empty/omitted; place the comment immediately above the SourcePullspecStyle field so godoc/tools pick it up and users of OpenshiftMappingConfig can see the allowed values and default.cmd/promoted-image-governor/main_test.go (1)
244-262: ⚡ Quick winAdd a compatibility test for empty style fallback to quay-proxy.
Please add one case where
SourcePullspecStyleis empty andSourceRegistry == api.QCIAPPCIDomain, asserting the same quay-proxy source format. This locks the intended backward-compatible behavior for existing mapping consumers.🤖 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 `@cmd/promoted-image-governor/main_test.go` around lines 244 - 262, Add a new table test entry in main_test.go mirroring the "source_pullspec_style quay-proxy" case but with OpenshiftMappingConfig.SourcePullspecStyle set to an empty string and SourceRegistry set to api.QCIAPPCIDomain; keep promotedTags and Images the same and assert the expected mapping (the "mapping_origin_4_8" key mapping to "quay-proxy.ci.openshift.org/openshift/ci:origin_4.8_bar" -> "quay.io/openshift/origin-bar:4.8") so the test verifies that an empty SourcePullspecStyle falls back to quay-proxy behavior for api.QCIAPPCIDomain.
🤖 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 `@cmd/promoted-image-governor/main.go`:
- Around line 238-250: The openshiftMappingSourcePullSpec currently silently
falls back for unknown mappingConfig.SourcePullspecStyle values; change it to
validate allowed styles and surface an error: update
openshiftMappingSourcePullSpec to return (string, error), check
mappingConfig.SourcePullspecStyle (normalized) against the allowed set
("registry-path"/"registry_path", "quay-proxy"/"quay_proxy", or empty only when
mappingConfig.SourceRegistry == api.QCIAPPCIDomain), return a descriptive error
(e.g., fmt.Errorf) if the style is unrecognized, and adjust all callers to
handle the returned error; preserve existing formatting logic using
mappingConfig.SourceRegistry/SourceNamespace and api.QuayImageReference when the
style is valid.
---
Nitpick comments:
In `@cmd/promoted-image-governor/main_test.go`:
- Around line 244-262: Add a new table test entry in main_test.go mirroring the
"source_pullspec_style quay-proxy" case but with
OpenshiftMappingConfig.SourcePullspecStyle set to an empty string and
SourceRegistry set to api.QCIAPPCIDomain; keep promotedTags and Images the same
and assert the expected mapping (the "mapping_origin_4_8" key mapping to
"quay-proxy.ci.openshift.org/openshift/ci:origin_4.8_bar" ->
"quay.io/openshift/origin-bar:4.8") so the test verifies that an empty
SourcePullspecStyle falls back to quay-proxy behavior for api.QCIAPPCIDomain.
In `@cmd/promoted-image-governor/main.go`:
- Around line 230-236: Add a Go doc comment for the exported field
SourcePullspecStyle on the OpenshiftMappingConfig struct describing the accepted
string values (e.g., "legacy", "ocp", or whatever the code expects), the
semantics of each value, and the default behavior when the field is
empty/omitted; place the comment immediately above the SourcePullspecStyle field
so godoc/tools pick it up and users of OpenshiftMappingConfig can see the
allowed values and default.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a12b125b-c478-4776-991c-f243792c4cc0
📒 Files selected for processing (2)
cmd/promoted-image-governor/main.gocmd/promoted-image-governor/main_test.go
deepsm007
force-pushed
the
governor-openshift-mapping-pullspec
branch
from
f26d18e to
32195e7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/promoted-image-governor/main_test.go (1)
378-380: ⚡ Quick winAssert the validation reason, not only error presence.
This currently passes on any
loadMappingConfigfailure. Assert that the error is specifically aboutsource_pullspec_styleso the test protects the intended contract.Suggested tightening
import ( "context" "fmt" "os" "path/filepath" "regexp" + "strings" "testing" @@ - if _, err := loadMappingConfig(path); err == nil { - t.Fatal("expected error") + if _, err := loadMappingConfig(path); err == nil || !strings.Contains(err.Error(), "source_pullspec_style") { + t.Fatalf("expected source_pullspec_style validation error, got: %v", err) }As per coding guidelines, "Ensure error messages are informative enough for developers to understand the error".
🤖 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 `@cmd/promoted-image-governor/main_test.go` around lines 378 - 380, The test currently only checks that loadMappingConfig(path) returns an error; instead assert the error message specifically indicates a validation failure for the source_pullspec_style field. Update the test around the loadMappingConfig call to verify err != nil and that err.Error() contains (or matches) a message referencing "source_pullspec_style" (or the exact validation text your loader emits), using the test helper/assert function you prefer so this test fails only when the source_pullspec_style contract is broken.
🤖 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.
Nitpick comments:
In `@cmd/promoted-image-governor/main_test.go`:
- Around line 378-380: The test currently only checks that
loadMappingConfig(path) returns an error; instead assert the error message
specifically indicates a validation failure for the source_pullspec_style field.
Update the test around the loadMappingConfig call to verify err != nil and that
err.Error() contains (or matches) a message referencing "source_pullspec_style"
(or the exact validation text your loader emits), using the test helper/assert
function you prefer so this test fails only when the source_pullspec_style
contract is broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dc42f89d-d435-42a7-92c7-82baa7a9bdf9
📒 Files selected for processing (2)
cmd/promoted-image-governor/main.gocmd/promoted-image-governor/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/promoted-image-governor/main.go
deepsm007
force-pushed
the
governor-openshift-mapping-pullspec
branch
from
32195e7 to
152394e
Compare
|
/test checkconfig
|
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
@deepsm007: all tests passed! 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. |
2 participants
/cc @openshift/test-platform
promoted-image-governor: Use quay-proxy-style source pullspecs in mapping generation and tighten error wrapping
Component affected:
What changed (practical impact for CI users/operators):
Why this matters:
Tests and validation:
Files with key changes:
No configuration schema additions (e.g., no new source_pullspec_style field) or other user-facing flags were added in this change.