Fix breaking changes #4834
Fix breaking changes #4834
Conversation
|
Pipeline controller notification For optional jobs, comment |
WalkthroughRemoved custom JSON (un)marshalling from secret bootstrap Config, switched Load/Save to strict YAML and deep-copy YAML marshaling, added ClusterGroups and DockerConfigJSONData fields, updated tests/fixtures and many secret routing YAMLs, and made small test/release and embedded YAML formatting tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (6)
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: 2
🧹 Nitpick comments (1)
pkg/api/secretbootstrap/secretboostrap.go (1)
72-108: Consider sorting the To field for complete determinism.The
MarshalYAMLmethod sorts theFrommap andDockerConfigJSONDataslices to ensure deterministic output, but theTofield (a[]SecretContextslice) is not sorted. If the order ofToelements can vary between runs, this could still produce non-deterministic YAML output.If complete determinism is required, consider sorting the
Toslice as well. For example:+ // Sort To slice for determinism + sortedTo := make([]SecretContext, len(s.To)) + copy(sortedTo, s.To) + sort.Slice(sortedTo, func(i, j int) bool { + if sortedTo[i].Cluster != sortedTo[j].Cluster { + return sortedTo[i].Cluster < sortedTo[j].Cluster + } + if sortedTo[i].Namespace != sortedTo[j].Namespace { + return sortedTo[i].Namespace < sortedTo[j].Namespace + } + return sortedTo[i].Name < sortedTo[j].Name + }) + return &secretConfigForMarshal{ From: sortedFrom, - To: s.To, + To: sortedTo, }, nilIf the current order of
Tois intentional and significant, please document why it's excluded from sorting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/api/secretbootstrap/secretboostrap.go(3 hunks)pkg/clusterinit/onboard/manifestgenerator.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/secretbootstrap/secretboostrap.gopkg/clusterinit/onboard/manifestgenerator.go
deepsm007
force-pushed
the
fix-breaking-changes
branch
from
a0551f8 to
e45d4b9
Compare
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, hector-vido The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
deepsm007
force-pushed
the
fix-breaking-changes
branch
from
e45d4b9 to
fbc882b
Compare
|
New changes are detected. LGTM label has been removed. |
deepsm007
force-pushed
the
fix-breaking-changes
branch
2 times, most recently
from
f547d5e to
96a3c50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/api/secretbootstrap/secretboostrap.go (1)
65-98: Avoid manual ItemContext reconstruction; clarify intent around map orderingThe DockerConfigJSONData sorting is good, but rebuilding
ItemContextfield‑by‑field is brittle: any future fields added toItemContextwill be silently dropped whenorderSecretConfigruns. Also, re‑creating theFrommap does not affect Go’s map iteration order, so it doesn’t buy determinism by itself.You can keep the behavior while making it safer and simpler by mutating the copied
ItemContextand writing it back into the existing map:-func (s *SecretConfig) orderSecretConfig() { - // Sort From map keys and DockerConfigJSONData slices - sortedFrom := make(map[string]ItemContext) - keys := make([]string, 0, len(s.From)) - for k := range s.From { - keys = append(keys, k) - } - sort.Strings(keys) - - for _, k := range keys { - itemCtx := s.From[k] - // Sort DockerConfigJSONData slice - dockerData := make([]DockerConfigJSONData, len(itemCtx.DockerConfigJSONData)) - copy(dockerData, itemCtx.DockerConfigJSONData) - sort.Slice(dockerData, func(i, j int) bool { - if dockerData[i].RegistryURL != dockerData[j].RegistryURL { - return dockerData[i].RegistryURL < dockerData[j].RegistryURL - } - if dockerData[i].Item != dockerData[j].Item { - return dockerData[i].Item < dockerData[j].Item - } - return dockerData[i].AuthField < dockerData[j].AuthField - }) - - sortedFrom[k] = ItemContext{ - Item: itemCtx.Item, - Field: itemCtx.Field, - DockerConfigJSONData: dockerData, - Base64Decode: itemCtx.Base64Decode, - } - } - s.From = sortedFrom -} +func (s *SecretConfig) orderSecretConfig() { + if len(s.From) == 0 { + return + } + + // Sort From keys to ensure we process entries in a stable order + keys := make([]string, 0, len(s.From)) + for k := range s.From { + keys = append(keys, k) + } + sort.Strings(keys) + + for _, k := range keys { + itemCtx := s.From[k] + + dockerData := make([]DockerConfigJSONData, len(itemCtx.DockerConfigJSONData)) + copy(dockerData, itemCtx.DockerConfigJSONData) + sort.Slice(dockerData, func(i, j int) bool { + if dockerData[i].RegistryURL != dockerData[j].RegistryURL { + return dockerData[i].RegistryURL < dockerData[j].RegistryURL + } + if dockerData[i].Item != dockerData[j].Item { + return dockerData[i].Item < dockerData[j].Item + } + return dockerData[i].AuthField < dockerData[j].AuthField + }) + + itemCtx.DockerConfigJSONData = dockerData + s.From[k] = itemCtx + } +}If your goal is also to make
From’s key order deterministic in serialized YAML/JSON, that will still require walkingkeysduring marshaling rather than relying on map iteration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/api/secretbootstrap/secretboostrap.go(4 hunks)pkg/clusterinit/onboard/manifestgenerator.go(1 hunks)pkg/image-graph-generator/images.go(4 hunks)pkg/image-graph-generator/manifests.go(2 hunks)test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/image-graph-generator/manifests.go
- pkg/image-graph-generator/images.go
- pkg/clusterinit/onboard/manifestgenerator.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yamlpkg/api/secretbootstrap/secretboostrap.go
🔇 Additional comments (2)
test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml (1)
9-9: YAML block scalar style change is appropriateSwitching from
|-to|keeps the content identical except for a trailing newline and aligns this expected fixture with the updated manifest generator output; no issues from a test or config perspective.pkg/api/secretbootstrap/secretboostrap.go (1)
118-166: Placement and scope of deterministic ordering look goodCentralizing deterministic ordering in
Config.orderConfig()(sortingClusterGroupsvalues,Secretsby firstTo’s cluster/namespace/name, andUserSecretsTargetClusters) and invoking it at the end ofresolve()is a clean approach that avoids custom marshal hooks while ensuring stable output order after all resolution logic has run. This structure looks sound and maintainable.Also applies to: 326-328
deepsm007
force-pushed
the
fix-breaking-changes
branch
4 times, most recently
from
e77f842 to
6254e03
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/image-graph-generator/images.go (1)
7-7: Deterministic ordering for parents/branches looks correctThe added
sort.Slicecalls onParentsandBranchesinUpdateImage,addImageRef, andupdateImageRefprovide stable, name/ID‑based ordering without altering the underlying graph semantics. Empty slices are handled safely, and the GraphQL inputs become reproducible across runs.If this pattern expands further, consider a small helper (e.g.,
sortParentsByName,sortBranchesByID) to centralize the ordering rules, but that’s optional.Also applies to: 88-97, 127-132, 138-144, 204-207, 213-219
pkg/api/secretbootstrap/secretboostrap.go (1)
21-35: Deterministic ordering for secretbootstrap config is sound; minor forward‑compat refactorThe new
orderSecretConfigandorderConfighelpers give you stable ordering for:
ClusterGroupskeys and their cluster lists,Secrets(grouped by firstToentry’s Cluster/Namespace/Name),UserSecretsTargetClusters,- and
DockerConfigJSONDataentries inside eachItemContext.Calling
c.orderConfig()fromresolve()centralizes this, and the added DockerConfigJSONData handling inresolve/stripVaultPrefixis consistent.One small maintainability tweak: in
orderSecretConfigyou manually copyItemContextfields, which means new fields added later could be accidentally dropped. You can keep behavior identical while future‑proofing with:- itemCtx := s.From[k] + itemCtx := s.From[k] // Sort DockerConfigJSONData slice dockerData := make([]DockerConfigJSONData, len(itemCtx.DockerConfigJSONData)) copy(dockerData, itemCtx.DockerConfigJSONData) sort.Slice(dockerData, func(i, j int) bool { // ... }) - - sortedFrom[k] = ItemContext{ - Item: itemCtx.Item, - Field: itemCtx.Field, - DockerConfigJSONData: dockerData, - Base64Decode: itemCtx.Base64Decode, - } + itemCtxCopy := itemCtx + itemCtxCopy.DockerConfigJSONData = dockerData + sortedFrom[k] = itemCtxCopyThis keeps all existing semantics but automatically preserves any future ItemContext fields.
Also applies to: 65-99, 118-166, 326-328
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
cmd/ci-secret-bootstrap/main_test.go(4 hunks)pkg/api/secretbootstrap/secretboostrap.go(4 hunks)pkg/api/secretbootstrap/secretboostrap_test.go(4 hunks)pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml(1 hunks)pkg/clusterinit/onboard/manifestgenerator.go(1 hunks)pkg/config/release_test.go(3 hunks)pkg/image-graph-generator/images.go(4 hunks)pkg/image-graph-generator/manifests.go(2 hunks)test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/image-graph-generator/manifests.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/image-graph-generator/images.gopkg/config/release_test.gopkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yamlpkg/clusterinit/onboard/manifestgenerator.gocmd/ci-secret-bootstrap/main_test.gotest/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yamlpkg/api/secretbootstrap/secretboostrap_test.gopkg/api/secretbootstrap/secretboostrap.go
🔇 Additional comments (5)
test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml (1)
9-9: Fixture update correctly reflects new scalar styleAligning
config.yamlwith|instead of|-matches the new manifest post-processing behavior and keeps the expectation deterministic; nothing else in the ConfigMap is affected.pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml (1)
1-59: Roundtrip fixture matches new secretbootstrap config shapeThe added secret_config using
cluster_groupsand the inclusion ofbuild02inuser_secrets_target_clustersare consistent with the newConfigsemantics and deterministic ordering (ClusterGroups + Secrets). The YAML stays structurally valid and aligns with the updated tests.pkg/config/release_test.go (1)
20-27: Robust commit hash extraction in testsUsing
isHexStringplus a 40‑character length check on each output line makes the test resilient to stray Git/hook noise while still guaranteeing a real hash is passed intof. Disabling hooks viacore.hooksPath /dev/nullfurther reduces flakiness. The helper and parsing logic look correct and self‑contained.Also applies to: 58-58, 71-86
pkg/api/secretbootstrap/secretboostrap_test.go (1)
52-55: Tests correctly capture new resolution and ordering semanticsThe expectations in
TestResolvingandTestLoadConfigFromFilenow match the resolved/orderedConfig:
Fromis normalized to empty maps where unset.ClusterGroupsis consistently represented as an empty map when absent.ItemContextvalues includeDockerConfigJSONData, even when empty, reflecting the new field.- The “basic base” test mirrors the updated fixture for mirror/rh‑cdn secrets and target clusters.
These updates look consistent with the implementation changes.
Also applies to: 92-100, 124-127, 174-209
cmd/ci-secret-bootstrap/main_test.go (1)
178-198: CLI tests updated coherently for new secretbootstrap behaviorThe adjusted fixtures and expectations in this file:
- Add the default
.dockerconfigjsonsecret (and associatedDockerConfigJSONData) todefaultConfig/defaultConfigWithoutDefaultCluster.- Reflect the new normalized/ordered shapes of
ConfigandItemContext(empty ClusterGroups maps, explicit empty DockerConfigJSONData slices).- Update error strings (e.g., in
TestConstructSecrets,TestGetUnusedItems,TestConstructDockerConfigJSON) to include the new dockerconfigjson‑related paths and deterministic aggregation order.These changes keep the CLI tests in sync with the API changes and deterministic ordering; no functional or robustness issues stand out.
Also applies to: 201-235, 252-265, 393-403, 1023-1028, 1867-1969, 2089-2126
| // Fix YAML scalar style for monitoring config: change trailing newline handling | ||
| // yaml.v2 doesn't support controlling scalar style, so we post-process it. | ||
| // We change "config.yaml: |-" (literal block scalar that strips trailing newline) to | ||
| // "config.yaml: |" (literal block scalar that preserves trailing newline). | ||
| // This is safe and targeted: only runs for the cluster-monitoring-config ConfigMap. | ||
| if kind, ok := manifestMap["kind"].(string); ok && kind == "ConfigMap" { | ||
| if name, ok := manifestMap["metadata"].(map[string]interface{})["name"].(string); ok && name == "cluster-monitoring-config" { | ||
| manifestBytesPatched = bytes.ReplaceAll(manifestBytesPatched, []byte("config.yaml: |-"), []byte("config.yaml: |")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard metadata cast to avoid potential panics
The new post‑processing is correctly scoped to kind: ConfigMap and metadata.name == "cluster-monitoring-config", but this line is unsafe:
if name, ok := manifestMap["metadata"].(map[string]interface{})["name"].(string); ok && name == "cluster-monitoring-config" {If manifestMap["metadata"] is absent or not a map[string]interface{}, this will panic before the ok check. That’s unlikely in normal manifests but turns malformed input into a hard crash.
Wrap the metadata lookup in its own type assertion to keep the step robust:
- if kind, ok := manifestMap["kind"].(string); ok && kind == "ConfigMap" {
- if name, ok := manifestMap["metadata"].(map[string]interface{})["name"].(string); ok && name == "cluster-monitoring-config" {
- manifestBytesPatched = bytes.ReplaceAll(manifestBytesPatched, []byte("config.yaml: |-"), []byte("config.yaml: |"))
- }
- }
+ if kind, ok := manifestMap["kind"].(string); ok && kind == "ConfigMap" {
+ if metadata, ok := manifestMap["metadata"].(map[string]interface{}); ok {
+ if name, ok := metadata["name"].(string); ok && name == "cluster-monitoring-config" {
+ manifestBytesPatched = bytes.ReplaceAll(manifestBytesPatched, []byte("config.yaml: |-"), []byte("config.yaml: |"))
+ }
+ }
+ }This preserves the current behavior while avoiding panics on unexpected manifest shapes.
🤖 Prompt for AI Agents
In pkg/clusterinit/onboard/manifestgenerator.go around lines 130 to 139, the
metadata name lookup does an inline type assertion that can panic if
manifestMap["metadata"] is missing or not a map; extract manifestMap["metadata"]
into a variable with a separate type assertion (e.g., meta, ok :=
manifestMap["metadata"].(map[string]interface{})), check ok before accessing
meta["name"], then assert name, ok := meta["name"].(string) and compare to
"cluster-monitoring-config"; this preserves current behavior while avoiding a
potential panic on malformed manifests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/ci-secret-bootstrap/main_test.go (1)
1023-1028: Updated aggregated error expectation for missing fields remains accurate but brittleThe new
expectedErrornow correctly accounts for the additional.dockerconfigjsonsecret (missingquay.iodata) alongside the original"key-name-1"missing-field error, matching the behavior after adding the default Docker config secret. If future Vault error formatting changes become noisy for this test, consider asserting on key substrings or using a helper to normalize error text, but that's optional for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
cmd/ci-secret-bootstrap/main_test.go(4 hunks)pkg/api/secretbootstrap/secretboostrap.go(4 hunks)pkg/api/secretbootstrap/secretboostrap_test.go(4 hunks)pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml(1 hunks)pkg/clusterinit/onboard/manifestgenerator.go(1 hunks)pkg/config/release_test.go(3 hunks)pkg/image-graph-generator/images.go(4 hunks)pkg/image-graph-generator/manifests.go(2 hunks)test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/secretbootstrap/secretboostrap.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/config/release_test.gopkg/image-graph-generator/images.gopkg/clusterinit/onboard/manifestgenerator.gopkg/api/secretbootstrap/secretboostrap_test.gopkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yamlcmd/ci-secret-bootstrap/main_test.gotest/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yamlpkg/image-graph-generator/manifests.go
🔇 Additional comments (12)
pkg/clusterinit/onboard/manifestgenerator.go (1)
130-139: LGTM! Post-processing workaround is safe and well-documented.The YAML scalar style adjustment is correctly targeted to the cluster-monitoring-config ConfigMap with appropriate guard conditions. The updated comments accurately describe the change. Looking forward to the follow-up PR for a more robust solution using yaml.v3's scalar style control as discussed.
test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml (1)
9-9: LGTM! Test expectation correctly reflects the scalar style change.The change from
|-to|matches the post-processing logic in manifestgenerator.go and correctly adjusts the expected YAML scalar style for the monitoring configuration.pkg/image-graph-generator/manifests.go (1)
161-164: LGTM! Deterministic ordering correctly implemented.The sorting of Parents by Name ensures stable ordering in manifest generation, preventing non-deterministic output that could cause breaking changes. The placement right before adding/updating the ImageRef is appropriate.
pkg/image-graph-generator/images.go (3)
88-96: LGTM! Deterministic ordering ensures stable graph operations.The sorting of Parents by Name and Branches by ID in UpdateImage prevents non-deterministic output and aligns with the PR's goal of fixing breaking changes.
127-143: LGTM! Proper use of sorted copies for deterministic payload construction.Creating sorted copies of Branches and Parents ensures the mutation payload has deterministic ordering without mutating the input slices. The approach is consistent with updateImageRef.
204-218: LGTM! Consistent deterministic ordering in update operations.The sorting strategy matches addImageRef, ensuring consistent behavior across add and update paths. Using sorted copies maintains proper encapsulation.
pkg/config/release_test.go (3)
20-27: LGTM: Hex validation helper is correct.The implementation correctly validates hexadecimal characters and serves its purpose well in filtering the commit hash from git output.
58-58: LGTM: Disabling hooks improves test isolation.Setting
core.hooksPathto/dev/nullprevents git hooks from interfering with test execution, which directly addresses the breaking changes this PR aims to fix.
71-86: LGTM: Robust commit hash extraction.The new parsing logic correctly filters git output to extract the commit hash, making the tests resilient to pre-commit hook output or other noise. The 40-character length check is appropriate for SHA-1 hashes (Git's current default), and the clear error message will aid debugging if issues arise.
pkg/api/secretbootstrap/secretboostrap_test.go (1)
52-52: Explicit zero-value expectations for new fields look correctThe updates to include
From: map[string]ItemContext{},ClusterGroups: map[string][]string{}, andDockerConfigJSONData: []DockerConfigJSONData{}in the expected configs align the tests with the new public fields and deterministic marshalling behavior ofConfig. This keeps the equality checks precise without changing semantics.Also applies to: 92-99, 125-126, 178-187
pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml (1)
38-54: Roundtrip fixture update matches new secret routing modelThe additional
secret_configsentry forops-mirror.pem/rh-cdn.pemkeyed bycluster_groups(build_farm,cg_2,cg_3) is structurally consistent with the existing YAML and the Go tests that consume it. This should keep the roundtrip test aligned with the new configuration behavior.cmd/ci-secret-bootstrap/main_test.go (1)
178-249: Default configs and group-resolution expectations are consistent with new DockerConfig handlingThe introduction of:
- an explicit
ClusterGroups: map[string][]string{}field,- a dedicated
.dockerconfigjsonsecret indefaultConfig(matching theconfigContentYAML), and- empty
DockerConfigJSONDataslices on allFromentries (including the "group is resolved" case)keeps the tests aligned with the updated
secretbootstrap.Configshape and Docker config semantics. The resolvedToentry retainingClusterGroupswhile settingCluster: "default"also matches the resolution behavior exercised in the package-level tests.Also applies to: 251-301, 395-403
deepsm007
force-pushed
the
fix-breaking-changes
branch
2 times, most recently
from
3d7301b to
e198410
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/release_test.go (1)
58-67: Simplify the fallback error handling logic.The fallback from
Output()toCombinedOutput()is confusing. Ifp.Output()fails,p.CombinedOutput()(which executes the same command) is unlikely to succeed. This nested error handling adds complexity without clear benefit.Consider simplifying to either:
- Use
Output()directly and handle errors, or- Use
CombinedOutput()if stderr is needed for debuggingApply this diff to simplify:
- // Use Output() instead of CombinedOutput() to avoid pre-commit hook output on stderr - out, err := p.Output() + out, err := p.Output() if err != nil { - // If Output() fails, try CombinedOutput() for error details - combinedOut, combinedErr := p.CombinedOutput() - if combinedErr != nil { - t.Fatalf("%q failed, output:\n%s", p.Args, combinedOut) - } - out = combinedOut + t.Fatalf("%q failed: %v", p.Args, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
cmd/ci-secret-bootstrap/main_test.go(4 hunks)pkg/api/secretbootstrap/secretboostrap.go(3 hunks)pkg/api/secretbootstrap/secretboostrap_test.go(4 hunks)pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml(1 hunks)pkg/clusterinit/onboard/manifestgenerator.go(1 hunks)pkg/config/release_test.go(2 hunks)test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml
- pkg/api/secretbootstrap/secretboostrap.go
- cmd/ci-secret-bootstrap/main_test.go
- pkg/clusterinit/onboard/manifestgenerator.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/api/secretbootstrap/secretboostrap_test.gopkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yamlpkg/config/release_test.go
🔇 Additional comments (3)
pkg/config/release_test.go (1)
49-49: Verify whether disabling git hooks is the appropriate solution.A past reviewer questioned whether these changes are necessary. While disabling hooks via
core.hooksPathand switching toOutput()addresses stderr contamination, consider whether:
- The test environment should have hooks that interfere with output
- A different approach (e.g., cleaning the test environment) might be more appropriate
Based on past review comments.
Also applies to: 58-71
pkg/api/secretbootstrap/secretboostrap_test.go (1)
52-52: LGTM!The test expectations have been correctly updated to reflect the new struct fields (
From,ClusterGroups,DockerConfigJSONData). Explicitly setting these fields tonilin test cases ensures accurate validation of the config resolution logic.Also applies to: 93-99, 125-125, 177-187
pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml (1)
38-54: LGTM!The test fixture has been correctly updated to reflect the new per-cluster-group routing structure. The secret configuration now properly maps
ops-mirror.pemandrh-cdn.pemto their target cluster groups (build_farm,cg_2,cg_3) with appropriate namespaces.
deepsm007
force-pushed
the
fix-breaking-changes
branch
2 times, most recently
from
104bf58 to
2fa1a04
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/config/release_test.go (1)
58-67: Cannot reuse exec.Cmd after Output() has been called.The fallback logic on lines 62-66 attempts to call
CombinedOutput()afterOutput()has already been invoked on the sameexec.Cmdobject. Both methods internally callRun(), and Go'sexec.Cmdcannot be reused once it has been started. This code will not execute as intended and may panic or produce unexpected behavior.Apply this diff to fix the issue by using
CombinedOutput()directly:- // Use Output() instead of CombinedOutput() to avoid pre-commit hook output on stderr - out, err := p.Output() + out, err := p.CombinedOutput() if err != nil { - // If Output() fails, try CombinedOutput() for error details - combinedOut, combinedErr := p.CombinedOutput() - if combinedErr != nil { - t.Fatalf("%q failed, output:\n%s", p.Args, combinedOut) - } - out = combinedOut + t.Fatalf("%q failed, output:\n%s", p.Args, out) }
🧹 Nitpick comments (2)
pkg/config/release_test.go (1)
49-49: Hook output concern is mitigated by line 49.Since
core.hooksPathis set to/dev/nullon line 49, git hooks are already disabled. The distinction betweenOutput()andCombinedOutput()to avoid hook stderr output is unnecessary. UsingCombinedOutput()directly would simplify the code and provide better error diagnostics when the git command fails.Also applies to: 58-58
pkg/api/secretbootstrap/secretboostrap.go (1)
73-88: Deterministic ordering + deep-copying ofConfiglooks solid overallUsing a deep copy in
SaveConfigToFileand applyingorderConfigonly to the copy preserves the caller’s view ofConfigwhile giving you stable, normalized output for YAML and tests, which is a nice property for an exported helper.
orderConfigitself is nil‑safe and gives you a consistent normalization pass:
- Cluster group member lists (
[]string) are copied and sorted.Secretsare sorted by the firstToentry’sCluster/Namespace/Name, with sensible handling of emptyToslices.UserSecretsTargetClustersand eachDockerConfigJSONDataslice are sorted, which removes slice-order nondeterminism in both top‑level and nested structures.One thing to keep in mind for future maintainers: rebuilding
ClusterGroupsvia sorted keys does not make map iteration deterministic (Go maps are unordered regardless of insertion order), so this normalization is mainly useful for struct equality and for having sorted per‑group member lists, not for relying on any particular map iteration order elsewhere. If you ever need deterministic traversal of cluster groups, you’ll still want to carry an explicit sorted key slice at call sites.Also applies to: 90-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
cmd/ci-secret-bootstrap/main_test.go(4 hunks)pkg/api/secretbootstrap/secretboostrap.go(2 hunks)pkg/api/secretbootstrap/secretboostrap_test.go(5 hunks)pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml(1 hunks)pkg/clusterinit/onboard/openshiftmonitoring.go(1 hunks)pkg/config/release_test.go(2 hunks)test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml(1 hunks)test/integration/cluster-init/update-build99/expected/core-services/ci-secret-bootstrap/_config.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/ci-secret-bootstrap/main_test.gopkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yamlpkg/api/secretbootstrap/secretboostrap.gopkg/config/release_test.gopkg/api/secretbootstrap/secretboostrap_test.gopkg/clusterinit/onboard/openshiftmonitoring.gotest/integration/cluster-init/update-build99/expected/core-services/ci-secret-bootstrap/_config.yaml
🧬 Code graph analysis (2)
cmd/ci-secret-bootstrap/main_test.go (1)
pkg/api/secretbootstrap/secretboostrap.go (4)
SecretConfig(59-62)ItemContext(20-27)DockerConfigJSONData(29-34)SecretContext(45-53)
pkg/api/secretbootstrap/secretboostrap.go (1)
pkg/api/types.go (1)
Cluster(749-749)
🔇 Additional comments (7)
pkg/clusterinit/onboard/openshiftmonitoring.go (1)
138-139: LGTM!The formatting adjustment to the raw string literal's closing backtick has no functional impact.
cmd/ci-secret-bootstrap/main_test.go (1)
179-402: LGTM! Test fixtures correctly updated for new API structure.The changes systematically update test data to reflect the new
ClusterGroupsandDockerConfigJSONDatafields introduced in the public API. Key updates include:
- Adding the
.dockerconfigjsonsecret as the first entry indefaultConfig(shifting the original secret from index 0 to 1)- Initializing
DockerConfigJSONDatato empty slices throughoutItemContextstructs for consistency- Correctly updating error messages to reference
config.1instead ofconfig.0for the main secret, since the new.dockerconfigjsonsecret is now at index 0The error message at lines 1023-1028 correctly captures two failures: the missing
quay.ioitem for the newconfig.0entry and the missing field initem-name-1forconfig.1.Also applies to: 1023-1028
pkg/config/release_test.go (1)
68-71: LGTM!Parsing and validating the commit hash to be exactly 40 characters improves test robustness and ensures that only the hash (not extra whitespace or output) is passed to the callback.
pkg/api/secretbootstrap/secretboostrap_test.go (2)
52-99: Updated expectations aroundClusterGroupsandDockerConfigJSONDataare consistent with the new APIThe adjustments in
TestResolvingandTestLoadConfigFromFileto:
- Declare
ClusterGroups: nilexplicitly,- Include
From: nilwhere appropriate, and- Populate
DockerConfigJSONData: nilinItemContextexpectations,all line up with the extended
Config/ItemContextshape and make the tests resilient to future ordering/normalization changes. The DPTP prefix tests correctly assert that the prefix is applied both toFrom.Itemand to eachDockerConfigJSONData[i].Item.Also applies to: 92-107, 124-127, 172-187
259-275: Round‑trip test now validates semantic equality viaorderConfig—good improvementSwitching
TestRoundtripConfigto:
- Load both the canonical fixture and the generated output into
Configstructs,- Normalize both via
orderConfig(), and- Compare via
cmp.Diff,is a solid way to verify round‑tripping without being brittle to field ordering in YAML. This directly exercises
SaveConfigToFile’s normalization behavior and keeps the test focused on logical equality of configs rather than raw file bytes.pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml (1)
38-55: Round‑trip fixture wiring for mirror/test secrets matches the new routing modelThe added
secret_configsentry forops-mirror.pem/rh-cdn.pem:
- Uses
fromitems and fields consistent withItemContext,- Targets
cluster_groups(build_farm,cg_2,cg_3) rather than hard‑coding clusters,which aligns with the updated
ClusterGroupsand resolving logic exercised inTestRoundtripConfig. Structurally this is compatible with the Go types and supports the new deterministic round‑trip behavior.test/integration/cluster-init/update-build99/expected/core-services/ci-secret-bootstrap/_config.yaml (1)
30-46: Restructured secret mappings for build99 look coherent and type‑consistentThe updated
_config.yaml:
- Groups related
sa.*entries under clearer logical items (e.g.,ci-chat-bot,cluster-display,pod-scaler) while keepingfieldvalues aligned with the actual key names.- Points
toblocks at appropriately renamed secrets (ci-chat-bot-kubeconfigs,cluster-display,dptp-controller-manager,github-ldap-user-group-creator,hook,pod-scaler,prow-controller-manager,sinker) in the expectedcinamespace.- Uses
from: {}for the empty registry‑pull config, which deserializes cleanly as an empty map and is compatible with themap[string]ItemContexttype.From a type/structure perspective this fixture matches the Go API and the updated routing model; no obvious miswirings or schema issues stand out in the changed sections.
Also applies to: 80-99, 121-151, 172-203, 218-236, 238-257, 257-257
deepsm007
force-pushed
the
fix-breaking-changes
branch
from
2fa1a04 to
d78043d
Compare
deepsm007
force-pushed
the
fix-breaking-changes
branch
from
d78043d to
e642ad5
Compare
|
@deepsm007: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Does it even help? The breaking-changes job is still failing |
| type configWithoutUnmarshaler Config | ||
|
|
||
| func (c *Config) UnmarshalJSON(d []byte) error { | ||
| var target configWithoutUnmarshaler | ||
| if err := json.Unmarshal(d, &target); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| *c = Config(target) | ||
| return c.resolve() | ||
| } | ||
|
|
||
| func (c *Config) MarshalJSON() ([]byte, error) { | ||
| target := &configWithoutUnmarshaler{ | ||
| VaultDPTPPrefix: c.VaultDPTPPrefix, | ||
| ClusterGroups: c.ClusterGroups, | ||
| UserSecretsTargetClusters: c.UserSecretsTargetClusters, | ||
| } | ||
| pre := c.VaultDPTPPrefix + "/" | ||
| var secrets []SecretConfig | ||
| for _, s := range c.Secrets { | ||
| var secret SecretConfig | ||
| if err := deepcopy.Copy(&secret, s); err != nil { | ||
| return nil, err | ||
| } | ||
| stripVaultPrefix(&secret, pre) | ||
| secret.groupClusters() | ||
| secrets = append(secrets, secret) | ||
| } | ||
|
|
||
| target.Secrets = secrets | ||
| return json.Marshal(target) | ||
| } | ||
|
|
There was a problem hiding this comment.
I need to push the latest changes, i havent had time to verify locally yet
| if err := yaml.UnmarshalStrict(bytes, config); err != nil { | ||
| return err | ||
| } | ||
| return config.resolve() |
5 participants
https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_ci-tools/4831/pull-ci-openshift-ci-tools-main-breaking-changes/1991097703345426432
/cc @openshift/test-platform