Fix breaking changes

[deepsm007]

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

[openshift-ci-robot]

Pipeline controller notification This repository is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. Review these jobs and use /test <job> to manually trigger optional jobs most likely to be impacted by the proposed changes.

[coderabbitai[bot]]

Walkthrough

Removed 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

Cohort / File(s)	Summary
Secret bootstrap core pkg/api/secretbootstrap/secretboostrap.go	Deleted MarshalJSON/UnmarshalJSON and stripVaultPrefix; removed encoding/json and strings imports. LoadConfigFromFile now uses gzip.ReadFileMaybeGZIP, yaml.UnmarshalStrict and returns config.resolve(). SaveConfigToFile deep-copies Config, marshals with YAML, and writes file. resolve() updated for Vault prefixing and cluster-group expansion.
Public API & tests cmd/ci-secret-bootstrap/main_test.go, pkg/api/secretbootstrap/secretboostrap_test.go, pkg/api/secretbootstrap/testdata/*	Added ClusterGroups map[string][]string to Config and DockerConfigJSONData []... to ItemContext. Tests updated to initialize/normalize these fields; added deterministic normalization (sorting) for comparisons; test fixtures updated accordingly.
Test fixtures & integration configs pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml, test/integration/.../openshift-monitoring/cluster-monitoring-config.yaml, test/integration/.../core-services/ci-secret-bootstrap/_config.yaml	Large rework of secret_configs routing to per-cluster-group mappings and many from/to remappings; added/updated cluster targets (e.g., build02). One YAML block scalar indicator/formatting change in monitoring ConfigMap.
Release test utility pkg/config/release_test.go	During tests set git config core.hooksPath /dev/null; prefer Output() with fallback to CombinedOutput() for errors; trim/validate commit hash and pass validated hash to callback.
OpenShift monitoring formatting pkg/clusterinit/onboard/openshiftmonitoring.go	Minor repositioning of raw-string backtick around embedded YAML (formatting only).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Areas needing extra attention:
  • Serialization/compatibility changes in pkg/api/secretbootstrap/secretboostrap.go (removed JSON hooks, YAML strict unmarshal, deep-copy semantics) and impact on callers.
  • resolve() modifications for Vault prefixing and cluster-group expansion — verify logic across varied fixture cases.
  • Public API additions (ClusterGroups, DockerConfigJSONData) propagation and initialization across tests and production code paths.
  • Large YAML fixture/config rework in test/integration/.../core-services/ci-secret-bootstrap/_config.yaml for correctness of secret routing.

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment

---

📜 Recent 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

📥 Commits

Reviewing files that changed from the base of the PR and between d78043d75e77f324b53492b8fa7ccfdeed9c974e and e642ad5db83b263b1e096972dfb706026b2df688.

📒 Files selected for processing (8)

• cmd/ci-secret-bootstrap/main_test.go (5 hunks)
• pkg/api/secretbootstrap/secretboostrap.go (1 hunks)
• pkg/api/secretbootstrap/secretboostrap_test.go (7 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 due to trivial changes (1)

• pkg/clusterinit/onboard/openshiftmonitoring.go

🚧 Files skipped from review as they are similar to previous changes (3)

• test/integration/cluster-init/update-build99/expected/clusters/build-clusters/build99/openshift-monitoring/cluster-monitoring-config.yaml
• cmd/ci-secret-bootstrap/main_test.go
• pkg/api/secretbootstrap/secretboostrap_test.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/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml
• pkg/config/release_test.go
• test/integration/cluster-init/update-build99/expected/core-services/ci-secret-bootstrap/_config.yaml
• pkg/api/secretbootstrap/secretboostrap.go

🔇 Additional comments (6)

pkg/config/release_test.go (1)

44-72: LGTM! Clean handling of git command output.

The change to use Output() instead of CombinedOutput() appropriately avoids pre-commit hook interference, and the fallback logic provides good error diagnostics. The commit hash validation ensures correctness.

pkg/api/secretbootstrap/testdata/zz_fixture_TestRoundtripConfig_basic_base.yaml (1)

38-54: Test fixture correctly reflects new cluster-group routing.

The new secret_configs entry with per-cluster-group routing aligns with the expanded API surface for ClusterGroups introduced in this PR.

pkg/api/secretbootstrap/secretboostrap.go (3)

68-71: LGTM! Strict YAML validation and normalization.

Using UnmarshalStrict catches configuration typos, and calling resolve() ensures cluster groups and Vault prefixes are properly expanded.

---

150-203: Cluster group expansion and Vault prefix logic is correct.

The resolve() method properly:

• Validates mutual exclusivity of cluster and cluster_groups
• Expands cluster groups to individual clusters
• Applies Vault prefix to items and DockerConfigJSON data

---

76-85: Correct the timeline and clarify findings; recommendation stands but is not urgent.

The github.com/getlantern/deepcopy dependency is indeed unmaintained, but the facts need correction: the last repository push was 2018-03-02, making it ~7 years old rather than 9 years. No security vulnerabilities were found in the GitHub advisory database.

While the library is old and receives no maintenance, it appears stable and is used as a direct dependency across multiple files (secretbootstrap, secretgenerator, shardprowconfig, diffs, jobconfig, rehearse, config-brancher). The recommendations to consider well-maintained alternatives like github.com/mitchellh/copystructure or JSON marshal/unmarshal for production code handling sensitive configuration remain valid—they reduce long-term maintenance risk—but this is not an urgent security concern given the absence of known vulnerabilities.

test/integration/cluster-init/update-build99/expected/core-services/ci-secret-bootstrap/_config.yaml (1)

257-275: Verify that empty from: {} is valid and intentional.

Line 257 defines a secret configuration with an empty from: {} block but valid to destinations. Confirm whether:

• The system supports secrets with no source data (e.g., for generated/placeholder secrets)
• This is an intentional test case for validation logic
• The config will pass validation in production

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[deepsm007]

/pipeline required

[openshift-ci-robot]

Scheduling required tests: /test e2e

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters: /test integration-optional-test

[hector-vido]

/lgtm

[openshift-ci-robot]

Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage.

[openshift-ci[bot]]

[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:

• ~~OWNERS~~ [deepsm007,hector-vido]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 472c3755eb2bf826c7566f37b00c529c32c9a2ca and 2 for PR HEAD e45d4b9a1aed713c364d0b41f2c7d66c1b12b50f in total

[deepsm007]

/hold

[openshift-ci[bot]]

New changes are detected. LGTM label has been removed.

[openshift-ci[bot]]

@deepsm007: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration-optional-test	e45d4b9a1aed713c364d0b41f2c7d66c1b12b50f	link	true	/test integration-optional-test
ci/prow/integration	e642ad5db83b263b1e096972dfb706026b2df688	link	true	/test integration
ci/prow/images	e642ad5db83b263b1e096972dfb706026b2df688	link	true	/test images
ci/prow/breaking-changes	e642ad5db83b263b1e096972dfb706026b2df688	link	false	/test breaking-changes
ci/prow/unit	e642ad5db83b263b1e096972dfb706026b2df688	link	true	/test unit

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.

[Prucek]

Does it even help? The breaking-changes job is still failing