Add default dptp collection in dockerconfigs in gsm-config.yaml#5136
Open
psalajova wants to merge 1 commit intoopenshift:mainfrom
Open
Add default dptp collection in dockerconfigs in gsm-config.yaml#5136psalajova wants to merge 1 commit intoopenshift:mainfrom
gsm-config.yaml#5136psalajova wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughMove docker registry collection config from per-registry Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (12 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
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
![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 (4)
pkg/steps/multi_stage/gsm_bundle_resolver_test.go (1)
247-247: Prefer the shared constant for collection literal in test fixtures.Use
api.DPTPGSMCollectioninstead of hardcoding"test-platform-infra"to avoid string drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go` at line 247, Replace the hardcoded collection literal "test-platform-infra" used in the test fixture's GSMDPTPCollection field with the shared constant api.DPTPGSMCollection; locate the struct initialiser that sets GSMDPTPCollection and substitute the string with api.DPTPGSMCollection to keep tests aligned with the canonical constant.pkg/api/gsm_test.go (1)
1328-1347: Add a negative test for invalidgsm_dptp_collectionformat.The suite now tests the “missing” case, but it should also assert failure for malformed collection names to cover the new validation branch.
As per coding guidelines "Be very careful with struct field tags, defaults, and validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm_test.go` around lines 1328 - 1347, Add a new negative table test case in the gsm_test suite that validates malformed gsm_dptp_collection names: create a GSMConfig that includes a GSMBundle using DockerConfigSpec (use RegistryAuthData and a TargetSpec with Type corev1.SecretTypeDockerConfigJson) and set the top-level gsm_dptp_collection field to an invalid format (e.g. a string that does not meet the expected collection name pattern). In that test case set expectError = true and errorContains to the validation message for bad format (similar to "gsm_dptp_collection must be set when bundles use dockerconfig" but reflecting the malformed-name message), so the test asserts that validation in the same code path that checks GSMConfig rejects malformed gsm_dptp_collection values.pkg/api/gsm.go (1)
24-27: Coordinate the API shape change with vendored consumers.
GSMConfig/RegistryAuthDatashape changed; linked-repo findings indicateopenshift/ci-chat-botvendors these types. Please plan a vendor bump there to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm.go` around lines 24 - 27, The public API types in pkg/api/gsm.go (notably GSMConfig and RegistryAuthData) changed shape—fields like ClusterGroups, GSMDPTPCollection, Components and Bundles were added/renamed—so coordinate a vendor bump for downstream consumers (openshift/ci-chat-bot) that vendor these types: open an issue/PR in that repo referencing this change, update its imports to the new GSMConfig/RegistryAuthData shape, bump the dependency to this new version, update vendored code (re-run dependency/vendoring step in their repo) and run their tests to verify compatibility; include a short migration note listing the renamed/added fields to help maintainers adapt.cmd/ci-secret-bootstrap/main.go (1)
460-460: Coordinate downstream vendored API update.This file now depends on the new GSM API contract (
GSMDPTPCollection+ no per-registryCollection). Please plan a vendor bump inopenshift/ci-chat-bot(vendor/github.com/openshift/ci-tools/pkg/api/gsm.goand deepcopy/validation files) to avoid cross-repo type drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ci-secret-bootstrap/main.go` at line 460, The constructDockerConfigJSONFromGSM function now relies on the new GSM API shape (GSMDPTPCollection and removal of per-registry Collection) so update downstream vendoring in openshift/ci-chat-bot: bump the vendor for github.com/openshift/ci-tools so vendor/.../pkg/api/gsm.go and its deepcopy/validation outputs match the new GSMDPTPCollection field (and absence of per-registry Collection); ensure any code that constructs gsmSecretRef or consumes api.RegistryAuthData aligns with the new types and rebuilds vendor to eliminate cross-repo type drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/gsm.go`:
- Around line 272-285: The validation currently treats c.GSMDPTPCollection as
mandatory when any bundle has DockerConfig; instead, if any bundle uses
DockerConfig (the hasDockerConfig loop), resolve the effective collection name
by defaulting to DPTPGSMCollection when c.GSMDPTPCollection is empty, and then
run gsmvalidation.ValidateCollectionName on that resolved name; remove the
unconditional append of the "must be set" error and only append an error if the
resolved collection name is invalid (use the same error message format that
currently references c.GSMDPTPCollection but substitute the resolved value).
---
Nitpick comments:
In `@cmd/ci-secret-bootstrap/main.go`:
- Line 460: The constructDockerConfigJSONFromGSM function now relies on the new
GSM API shape (GSMDPTPCollection and removal of per-registry Collection) so
update downstream vendoring in openshift/ci-chat-bot: bump the vendor for
github.com/openshift/ci-tools so vendor/.../pkg/api/gsm.go and its
deepcopy/validation outputs match the new GSMDPTPCollection field (and absence
of per-registry Collection); ensure any code that constructs gsmSecretRef or
consumes api.RegistryAuthData aligns with the new types and rebuilds vendor to
eliminate cross-repo type drift.
In `@pkg/api/gsm_test.go`:
- Around line 1328-1347: Add a new negative table test case in the gsm_test
suite that validates malformed gsm_dptp_collection names: create a GSMConfig
that includes a GSMBundle using DockerConfigSpec (use RegistryAuthData and a
TargetSpec with Type corev1.SecretTypeDockerConfigJson) and set the top-level
gsm_dptp_collection field to an invalid format (e.g. a string that does not meet
the expected collection name pattern). In that test case set expectError = true
and errorContains to the validation message for bad format (similar to
"gsm_dptp_collection must be set when bundles use dockerconfig" but reflecting
the malformed-name message), so the test asserts that validation in the same
code path that checks GSMConfig rejects malformed gsm_dptp_collection values.
In `@pkg/api/gsm.go`:
- Around line 24-27: The public API types in pkg/api/gsm.go (notably GSMConfig
and RegistryAuthData) changed shape—fields like ClusterGroups,
GSMDPTPCollection, Components and Bundles were added/renamed—so coordinate a
vendor bump for downstream consumers (openshift/ci-chat-bot) that vendor these
types: open an issue/PR in that repo referencing this change, update its imports
to the new GSMConfig/RegistryAuthData shape, bump the dependency to this new
version, update vendored code (re-run dependency/vendoring step in their repo)
and run their tests to verify compatibility; include a short migration note
listing the renamed/added fields to help maintainers adapt.
In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go`:
- Line 247: Replace the hardcoded collection literal "test-platform-infra" used
in the test fixture's GSMDPTPCollection field with the shared constant
api.DPTPGSMCollection; locate the struct initialiser that sets GSMDPTPCollection
and substitute the string with api.DPTPGSMCollection to keep tests aligned with
the canonical constant.
🪄 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: b35b0588-75d6-4268-86d9-e5b017fd0782
📒 Files selected for processing (6)
cmd/ci-secret-bootstrap/main.gocmd/ci-secret-bootstrap/main_test.gocmd/ci-secret-generator/main_test.gopkg/api/gsm.gopkg/api/gsm_test.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
psalajova
force-pushed
the
gsm-config-add-dptp-collection
branch
from
d5ba199 to
4d65c7f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/api/gsm.go (1)
272-285:⚠️ Potential issue | 🟠 MajorDefault the effective dockerconfig collection instead of rejecting empty
dptp_collection.Line 280 currently hard-fails when
dptp_collectionis omitted. That removes zero-value compatibility and undermines the new default-collection behavior. Resolve the effective value first (DPTPGSMCollection), then validate it.Suggested fix
- if hasDockerConfig && c.DPTPCollection == "" { - errs = append(errs, fmt.Errorf("dptp_collection must be set when bundles use dockerconfig")) - } - if c.DPTPCollection != "" && !gsmvalidation.ValidateCollectionName(c.DPTPCollection) { - errs = append(errs, fmt.Errorf("dptp_collection has invalid collection name: %s", c.DPTPCollection)) - } + collection := c.DPTPCollection + if hasDockerConfig && collection == "" { + collection = DPTPGSMCollection + } + if collection != "" && !gsmvalidation.ValidateCollectionName(collection) { + errs = append(errs, fmt.Errorf("dptp_collection has invalid collection name: %s", collection)) + }As per coding guidelines "Backward compatibility matters — new fields should have zero-value defaults that preserve existing behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm.go` around lines 272 - 285, Compute the effective collection value (the resolved DPTPGSMCollection) before validating and only error if that effective value is empty when any bundle has DockerConfig; currently the code checks c.DPTPCollection directly and rejects zero-value, breaking default behavior. Update the block that sets hasDockerConfig and the subsequent checks to first derive the effective collection (using the same resolution logic that produces DPTPGSMCollection), then: if hasDockerConfig and effectiveCollection == "" append the error, and if effectiveCollection != "" run gsmvalidation.ValidateCollectionName on effectiveCollection (not c.DPTPCollection). Ensure you reference c.Bundles, bundle.DockerConfig, c.DPTPCollection and the effective DPTPGSMCollection variable when making the change.
🧹 Nitpick comments (2)
pkg/steps/multi_stage/gsm_bundle_resolver_test.go (1)
247-247: Use the exported default constant in the fixture.Line 247 hardcodes
"test-platform-infra". Preferapi.DPTPGSMCollectionto avoid drift.Suggested cleanup
- DPTPCollection: "test-platform-infra", + DPTPCollection: api.DPTPGSMCollection,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go` at line 247, The test fixture currently hardcodes the collection name "test-platform-infra" (in the DPTPCollection field); replace that literal with the exported constant api.DPTPGSMCollection so the test uses the canonical default value. Locate the fixture setting for DPTPCollection in gsm_bundle_resolver_test.go and change the value to api.DPTPGSMCollection, ensuring the api package is imported where necessary.pkg/api/gsm.go (1)
24-27: Coordinate this API change with downstream vendored consumers.
GSMConfig.DPTPCollectionaddition plusRegistryAuthData.Collectionremoval changes exported API shape. Please keep the downstream vendor/update (notablyopenshift/ci-chat-bot) coupled with this PR to avoid type/deepcopy drift.Also applies to: 77-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm.go` around lines 24 - 27, The added GSMConfig.DPTPCollection field and removal of RegistryAuthData.Collection change the public API shape; coordinate with downstream vendored consumers (e.g., openshift/ci-chat-bot) to prevent type/deepcopy drift by updating their types and generated deepcopy code to match these changes, bumping their vendored dependency or submitting a follow-up PR that consumes this change; ensure all uses of GSMConfig and RegistryAuthData in consumers are updated to read/write DPTPCollection and to stop relying on RegistryAuthData.Collection, and run/update codegen (deepcopy/genclient) and tests in both this repo and downstream to validate there are no compile or runtime incompatibilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/api/gsm.go`:
- Around line 272-285: Compute the effective collection value (the resolved
DPTPGSMCollection) before validating and only error if that effective value is
empty when any bundle has DockerConfig; currently the code checks
c.DPTPCollection directly and rejects zero-value, breaking default behavior.
Update the block that sets hasDockerConfig and the subsequent checks to first
derive the effective collection (using the same resolution logic that produces
DPTPGSMCollection), then: if hasDockerConfig and effectiveCollection == ""
append the error, and if effectiveCollection != "" run
gsmvalidation.ValidateCollectionName on effectiveCollection (not
c.DPTPCollection). Ensure you reference c.Bundles, bundle.DockerConfig,
c.DPTPCollection and the effective DPTPGSMCollection variable when making the
change.
---
Nitpick comments:
In `@pkg/api/gsm.go`:
- Around line 24-27: The added GSMConfig.DPTPCollection field and removal of
RegistryAuthData.Collection change the public API shape; coordinate with
downstream vendored consumers (e.g., openshift/ci-chat-bot) to prevent
type/deepcopy drift by updating their types and generated deepcopy code to match
these changes, bumping their vendored dependency or submitting a follow-up PR
that consumes this change; ensure all uses of GSMConfig and RegistryAuthData in
consumers are updated to read/write DPTPCollection and to stop relying on
RegistryAuthData.Collection, and run/update codegen (deepcopy/genclient) and
tests in both this repo and downstream to validate there are no compile or
runtime incompatibilities.
In `@pkg/steps/multi_stage/gsm_bundle_resolver_test.go`:
- Line 247: The test fixture currently hardcodes the collection name
"test-platform-infra" (in the DPTPCollection field); replace that literal with
the exported constant api.DPTPGSMCollection so the test uses the canonical
default value. Locate the fixture setting for DPTPCollection in
gsm_bundle_resolver_test.go and change the value to api.DPTPGSMCollection,
ensuring the api package is imported where necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 79308461-8c8b-416c-8753-ef52983a7546
📒 Files selected for processing (6)
cmd/ci-secret-bootstrap/main.gocmd/ci-secret-bootstrap/main_test.gocmd/ci-secret-generator/main_test.gopkg/api/gsm.gopkg/api/gsm_test.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/ci-secret-bootstrap/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ci-secret-generator/main_test.go
psalajova
force-pushed
the
gsm-config-add-dptp-collection
branch
from
4d65c7f to
c84051a
Compare
psalajova
force-pushed
the
gsm-config-add-dptp-collection
branch
from
c84051a to
7a666b5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/api/gsm.go (1)
281-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hard-failing empty
dptp_collectioninValidate()Line 289 introduces a validation failure for unresolved configs, even though Line 128 already defines a safe defaulting path. Validate the effective collection (defaulting to
DPTPGSMCollectionwhen dockerconfig is present) instead of requiring explicit input.Suggested adjustment
- if hasDockerConfig && c.DPTPCollection == "" { - errs = append(errs, fmt.Errorf("dptp_collection must be set when bundles use dockerconfig")) - } - if c.DPTPCollection != "" && !gsmvalidation.ValidateCollectionName(c.DPTPCollection) { - errs = append(errs, fmt.Errorf("dptp_collection has invalid collection name: %s", c.DPTPCollection)) + effectiveCollection := c.DPTPCollection + if hasDockerConfig && effectiveCollection == "" { + effectiveCollection = DPTPGSMCollection + } + if effectiveCollection != "" && !gsmvalidation.ValidateCollectionName(effectiveCollection) { + errs = append(errs, fmt.Errorf("dptp_collection has invalid collection name: %s", effectiveCollection)) }As per coding guidelines "Backward compatibility matters — new fields should have zero-value defaults that preserve existing behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm.go` around lines 281 - 294, The Validate() logic currently hard-fails when any bundle has DockerConfig and c.DPTPCollection is empty; instead compute the effective collection name (use c.DPTPCollection if non-empty, otherwise use the default constant DPTPGSMCollection when hasDockerConfig is true) and run gsmvalidation.ValidateCollectionName against that effective value; update the block that checks hasDockerConfig and the subsequent ValidateCollectionName call to validate and append errors based on the effectiveCollection variable rather than requiring an explicit DPTPCollection.
🧹 Nitpick comments (1)
pkg/api/gsm.go (1)
24-27: Plan downstream vendor sync before unholding this PR
GSMConfigshape changed (DPTPCollectionadded / registry collection moved). Please ensure linked consumers with vendored ci-tools API (notablyopenshift/ci-chat-bot) bump vendor and regenerate deepcopy code in lockstep to avoid cross-repo type/build drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/gsm.go` around lines 24 - 27, GSMConfig's shape changed (new DPTPCollection field and moved registry collection), so before unholding this PR coordinate a downstream vendor sync: notify linked consumers (e.g., openshift/ci-chat-bot) to update their vendored ci-tools API, run `go mod vendor`/vendor bump, and regenerate deepcopy code to match the new GSMConfig (fields like DPTPCollection, ClusterGroups, Components, Bundles, and GSMSecretRef) to prevent cross-repo type/build drift; ensure consumers compile and their generated deep-copy files are committed in the same change that updates the vendored API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/api/gsm.go`:
- Around line 281-294: The Validate() logic currently hard-fails when any bundle
has DockerConfig and c.DPTPCollection is empty; instead compute the effective
collection name (use c.DPTPCollection if non-empty, otherwise use the default
constant DPTPGSMCollection when hasDockerConfig is true) and run
gsmvalidation.ValidateCollectionName against that effective value; update the
block that checks hasDockerConfig and the subsequent ValidateCollectionName call
to validate and append errors based on the effectiveCollection variable rather
than requiring an explicit DPTPCollection.
---
Nitpick comments:
In `@pkg/api/gsm.go`:
- Around line 24-27: GSMConfig's shape changed (new DPTPCollection field and
moved registry collection), so before unholding this PR coordinate a downstream
vendor sync: notify linked consumers (e.g., openshift/ci-chat-bot) to update
their vendored ci-tools API, run `go mod vendor`/vendor bump, and regenerate
deepcopy code to match the new GSMConfig (fields like DPTPCollection,
ClusterGroups, Components, Bundles, and GSMSecretRef) to prevent cross-repo
type/build drift; ensure consumers compile and their generated deep-copy files
are committed in the same change that updates the vendored API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0bcb4d11-9a3c-4647-8d6c-9c42ba0e6ffd
📒 Files selected for processing (6)
cmd/ci-secret-bootstrap/main.gocmd/ci-secret-bootstrap/main_test.gocmd/ci-secret-generator/main_test.gopkg/api/gsm.gopkg/api/gsm_test.gopkg/steps/multi_stage/gsm_bundle_resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ci-secret-generator/main_test.go
- cmd/ci-secret-bootstrap/main_test.go
Contributor
Author
|
/test e2e |
Contributor
|
@psalajova: 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. |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, psalajova 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 |
Contributor
Author
|
/test e2e |
Contributor
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
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.
This should reduce the number of redundant lines in dockerconfgis defined in bundles in gsm-config.yaml, as we always use
test-platform-infracollection to store the necessary secrets used in dockerconfigs.Release repo follow up, after the image is propagated: openshift/release#78405
Summary by CodeRabbit
New Features
Validation
Refactor
Tests