determinize-ci-operator: add --validate-main-promotion for main/master promotion validation#5021
Closed
deepsm007 wants to merge 1 commit intoopenshift:mainfrom
Closed
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new main-promotion validation flow to the determinize-ci-operator CLI, new validation implementation in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007 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 |
openshift-ci
bot
added
the
approved
deepsm007
force-pushed
the
determinize-validate-main-promotion
branch
from
4e3718d to
b118197
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/mainpromotion/validate_test.go (1)
196-243: Add regression tests for the two critical policy edges inValidate.Current tests don’t pin behavior for (a) enabled main/master promotion targets in non-
ocp/ocp-privatenamespaces, and (b) malformed_prowconfig.yamlhandling. Adding both cases will lock in policy enforcement and prevent silent bypass regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate_test.go` around lines 196 - 243, Add two regression tests to validate_test.go that exercise the Validate function's policy edges: (1) create a temp config dir containing a promotion config with main/master promotion targets enabled in a namespace other than "ocp" or "ocp-private" and assert Validate(...) returns an error (use Validate as called in existing tests), and (2) create a temp config dir containing a malformed _prowconfig.yaml (invalid YAML) and assert Validate(...) returns an error; place these tests alongside the existing TestValidate_* cases and reuse the same temp-dir pattern and argument signatures so defaultIgnore and Validate behavior are pinned.
🤖 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/mainpromotion/validate.go`:
- Around line 181-184: The ignore list check (ignore.Has(orgRepo)) is only
applied inside the isMainOrMasterBranch(info.Branch) branch and not in the
release-branch validation, causing ignored repos to still be validated for
release branches; update validate.go so the ignore.Has(orgRepo) check is
performed before either branch-specific validation or duplicate the same check
at the start of the release-branch block (the logic surrounding
isMainOrMasterBranch(info.Branch) and the release-branch handling around lines
where release branch validation occurs) to short-circuit and return nil for
ignored orgRepo values in both paths.
- Around line 82-89: The function hasCurrentReleaseBranchInProw currently
swallows read/unmarshal errors (os.ReadFile and yaml.Unmarshal) and returns
false, hiding config problems; change hasCurrentReleaseBranchInProw to return
(bool, error) (or otherwise return an error up the call chain) and in the
os.ReadFile and yaml.Unmarshal error paths return a descriptive error (including
path and underlying err) instead of false; update its caller (the enforcement
check around line 209) to handle the error—log/propagate it and abort
enforcement rather than silently skipping—so unreadable or malformed
_prowconfig.yaml surfaces as an actual error.
- Around line 192-203: The current loop only rejects mismatched names for ns ==
"ocp" and ns == "ocp-private", letting targets in any other namespace pass;
update the validation in pkg/mainpromotion/validate.go to reject any namespace
other than "ocp" or "ocp-private". Concretely: keep the existing checks for ns
== "ocp" (compare name != release) and ns == "ocp-private" (compare name !=
releasePriv) and add an else branch that appends an error to errs for any other
ns (using relPath, ns and name) indicating main/master may only promote to the
allowed release streams; reference the variables ns, name, release, releasePriv,
errs and relPath when constructing the error message.
---
Nitpick comments:
In `@pkg/mainpromotion/validate_test.go`:
- Around line 196-243: Add two regression tests to validate_test.go that
exercise the Validate function's policy edges: (1) create a temp config dir
containing a promotion config with main/master promotion targets enabled in a
namespace other than "ocp" or "ocp-private" and assert Validate(...) returns an
error (use Validate as called in existing tests), and (2) create a temp config
dir containing a malformed _prowconfig.yaml (invalid YAML) and assert
Validate(...) returns an error; place these tests alongside the existing
TestValidate_* cases and reuse the same temp-dir pattern and argument signatures
so defaultIgnore and Validate behavior are pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5fcef679-1ff4-4b5e-8fb5-8438e0e5c06f
📒 Files selected for processing (3)
cmd/determinize-ci-operator/main.gopkg/mainpromotion/validate.gopkg/mainpromotion/validate_test.go
Comment on lines
+181
to
+184
| if isMainOrMasterBranch(info.Branch) { | ||
| if ignore.Has(orgRepo) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ignore list is not applied consistently to release-branch validation.
Ignored repos are skipped for main/master checks (Line 182), but still evaluated in the release-branch block (Line 207). This makes default/custom ignores partially ineffective.
Suggested fix
orgRepo := fmt.Sprintf("%s/%s", info.Org, info.Repo)
relPath := filepath.Join(info.Org, info.Repo, filepath.Base(info.Filename))
+ if ignore.Has(orgRepo) {
+ return nil
+ }
if isMainOrMasterBranch(info.Branch) {
- if ignore.Has(orgRepo) {
- return nil
- }
if cfg.PromotionConfiguration == nil {
return nil
}Also applies to: 207-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/mainpromotion/validate.go` around lines 181 - 184, The ignore list check
(ignore.Has(orgRepo)) is only applied inside the
isMainOrMasterBranch(info.Branch) branch and not in the release-branch
validation, causing ignored repos to still be validated for release branches;
update validate.go so the ignore.Has(orgRepo) check is performed before either
branch-specific validation or duplicate the same check at the start of the
release-branch block (the logic surrounding isMainOrMasterBranch(info.Branch)
and the release-branch handling around lines where release branch validation
occurs) to short-circuit and return nil for ignored orgRepo values in both
paths.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/mainpromotion/validate.go (3)
181-184:⚠️ Potential issue | 🟠 MajorApply ignore filtering before both branch-policy paths.
Ignored repositories are skipped for main/master checks but still evaluated in the release-branch block, so ignore behavior is only partial.
Suggested fix
orgRepo := fmt.Sprintf("%s/%s", info.Org, info.Repo) relPath := filepath.Join(info.Org, info.Repo, filepath.Base(info.Filename)) + if ignore.Has(orgRepo) { + return nil + } if isMainOrMasterBranch(info.Branch) { - if ignore.Has(orgRepo) { - return nil - } if cfg.PromotionConfiguration == nil { return nil }Also applies to: 207-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 181 - 184, The ignore.Has(orgRepo) check is only applied inside the isMainOrMasterBranch(info.Branch) branch, leaving ignored repos still processed by the release-branch logic; move or duplicate the ignore.Has(orgRepo) guard so it runs before any branch-policy handling: i.e., at the start of the branch decision in the validate function (before the isMainOrMasterBranch(...) and before the release-branch handling block around the code that processes release branches), return nil if ignore.Has(orgRepo) is true so both the isMainOrMasterBranch path and the release branch path (the block handling release branch checks around the existing release-branch code) skip ignored repositories.
80-89:⚠️ Potential issue | 🟠 MajorDo not suppress
_prowconfig.yamlread/parse failures.Returning
falseon read/unmarshal errors makes malformed or unreadable Prow config look “out of scope,” which skips enforcement instead of failing validation.Suggested fix
-func hasCurrentReleaseBranchInProw(prowConfigDir, org, repo, currentRelease string) bool { +func hasCurrentReleaseBranchInProw(prowConfigDir, org, repo, currentRelease string) (bool, error) { path := filepath.Join(prowConfigDir, org, repo, "_prowconfig.yaml") data, err := os.ReadFile(path) if err != nil { - return false + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("read prow config %s: %w", path, err) } var cfg prowConfig if err := yaml.Unmarshal(data, &cfg); err != nil { - return false + return false, fmt.Errorf("unmarshal prow config %s: %w", path, err) } @@ - return true + return true, nil } } } - return false + return false, nil } @@ - if isReleaseBranch(info.Branch, release) && o.reposWithMainMaster.Has(orgRepo) { - inScopeProw := prowConfigDir != "" && hasCurrentReleaseBranchInProw(prowConfigDir, info.Org, info.Repo, release) + if isReleaseBranch(info.Branch, release) && o.reposWithMainMaster.Has(orgRepo) { + inScopeProw := false + if prowConfigDir != "" { + var err error + inScopeProw, err = hasCurrentReleaseBranchInProw(prowConfigDir, info.Org, info.Repo, release) + if err != nil { + return err + } + } if !inScopeProw { return nil }Also applies to: 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 80 - 89, The function hasCurrentReleaseBranchInProw currently swallows read/unmarshal errors and returns false; change its signature to return (bool, error) and propagate any os.ReadFile or yaml.Unmarshal errors instead of treating them as "not found" so validation fails on unreadable/malformed _prowconfig.yaml; update all callers to handle the error (e.g., check and return/fail validation). Make the same change for the other helper that suppresses errors (the second occurrence referenced in the review) so both file read and parsing failures are returned upward rather than ignored.
192-203:⚠️ Potential issue | 🟠 MajorReject main/master promotions to non-allowed namespaces.
Current logic validates only
ocp/ocp-privatename mismatches; enabled targets in any other namespace pass without violation.Suggested fix
name := strings.Trim(t.Name, `"`) - if ns == "ocp" && name != release { - errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, release)) - } - if ns == "ocp-private" && name != releasePriv { - errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, releasePriv)) - } + switch ns { + case "ocp": + if name != release { + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, release)) + } + case "ocp-private": + if name != releasePriv { + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, releasePriv)) + } + default: + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to ocp/%s or ocp-private/%s)", relPath, ns, name, release, releasePriv)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 192 - 203, The current check only flags mismatched names for the allowed namespaces but ignores any targets in other namespaces; update the validation inside the loop that inspects t.Namespace/t.Name so that if ns is not "ocp" or "ocp-private" you append an error to errs rejecting promotions to that namespace (include relPath and the offending ns/name in the message), and keep the existing checks for "ocp" (compare name to release) and "ocp-private" (compare name to releasePriv) using the same err append pattern.
🧹 Nitpick comments (1)
pkg/mainpromotion/validate_test.go (1)
166-194: Add regression tests for policy-bypass paths.The new suite still misses cases that currently decide whether enforcement is skipped: malformed/unreadable
org/repo/_prowconfig.yaml, ignore-list behavior on release-branch validation, and main/master promotion to non-ocp/ocp-privatenamespaces. Please add explicitValidate(...)tests for these to prevent silent regressions in the enforcement logic.Also applies to: 231-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate_test.go` around lines 166 - 194, The test suite is missing regression tests for policy-bypass paths; add new unit tests that exercise hasCurrentReleaseBranchInProw and the top-level Validate(...) enforcement logic to cover: (1) malformed or unreadable org/repo/_prowconfig.yaml (create a file with invalid YAML or set unreadable perms and assert Validate and hasCurrentReleaseBranchInProw return the bypass/false behavior), (2) ignore-list behavior for release-branch validation (add repos/names in the ignore list and assert release checks are skipped), and (3) main/master promotion to non-ocp / non-ocp-private namespaces (call Validate with a main/master promotion target namespace and assert enforcement is skipped); use the functions hasCurrentReleaseBranchInProw and Validate as the entry points in tests and assert expected true/false or skipped-enforcement outcomes to prevent regressions.
🤖 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/mainpromotion/validate.go`:
- Around line 181-184: The ignore.Has(orgRepo) check is only applied inside the
isMainOrMasterBranch(info.Branch) branch, leaving ignored repos still processed
by the release-branch logic; move or duplicate the ignore.Has(orgRepo) guard so
it runs before any branch-policy handling: i.e., at the start of the branch
decision in the validate function (before the isMainOrMasterBranch(...) and
before the release-branch handling block around the code that processes release
branches), return nil if ignore.Has(orgRepo) is true so both the
isMainOrMasterBranch path and the release branch path (the block handling
release branch checks around the existing release-branch code) skip ignored
repositories.
- Around line 80-89: The function hasCurrentReleaseBranchInProw currently
swallows read/unmarshal errors and returns false; change its signature to return
(bool, error) and propagate any os.ReadFile or yaml.Unmarshal errors instead of
treating them as "not found" so validation fails on unreadable/malformed
_prowconfig.yaml; update all callers to handle the error (e.g., check and
return/fail validation). Make the same change for the other helper that
suppresses errors (the second occurrence referenced in the review) so both file
read and parsing failures are returned upward rather than ignored.
- Around line 192-203: The current check only flags mismatched names for the
allowed namespaces but ignores any targets in other namespaces; update the
validation inside the loop that inspects t.Namespace/t.Name so that if ns is not
"ocp" or "ocp-private" you append an error to errs rejecting promotions to that
namespace (include relPath and the offending ns/name in the message), and keep
the existing checks for "ocp" (compare name to release) and "ocp-private"
(compare name to releasePriv) using the same err append pattern.
---
Nitpick comments:
In `@pkg/mainpromotion/validate_test.go`:
- Around line 166-194: The test suite is missing regression tests for
policy-bypass paths; add new unit tests that exercise
hasCurrentReleaseBranchInProw and the top-level Validate(...) enforcement logic
to cover: (1) malformed or unreadable org/repo/_prowconfig.yaml (create a file
with invalid YAML or set unreadable perms and assert Validate and
hasCurrentReleaseBranchInProw return the bypass/false behavior), (2) ignore-list
behavior for release-branch validation (add repos/names in the ignore list and
assert release checks are skipped), and (3) main/master promotion to non-ocp /
non-ocp-private namespaces (call Validate with a main/master promotion target
namespace and assert enforcement is skipped); use the functions
hasCurrentReleaseBranchInProw and Validate as the entry points in tests and
assert expected true/false or skipped-enforcement outcomes to prevent
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4f8961d-1f34-4dd3-9f67-110b14821795
📒 Files selected for processing (3)
cmd/determinize-ci-operator/main.gopkg/mainpromotion/validate.gopkg/mainpromotion/validate_test.go
deepsm007
force-pushed
the
determinize-validate-main-promotion
branch
from
b118197 to
54cb6f0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/mainpromotion/validate.go (3)
193-203:⚠️ Potential issue | 🟠 MajorReject disallowed namespaces for main/master promotion targets
Line 198 and Line 201 only validate
ocpandocp-privatenames; targets in any other namespace currently pass without error.Suggested fix
name := strings.Trim(t.Name, `"`) - if ns == "ocp" && name != release { - errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, release)) - } - if ns == "ocp-private" && name != releasePriv { - errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, releasePriv)) - } + switch ns { + case "ocp": + if name != release { + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, release)) + } + case "ocp-private": + if name != releasePriv { + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to %s)", relPath, ns, name, releasePriv)) + } + default: + errs = append(errs, fmt.Errorf("%s: promotes to %s/%s (main/master must only promote to ocp/%s or ocp-private/%s)", relPath, ns, name, release, releasePriv)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 193 - 203, The current validation only checks the "ocp" and "ocp-private" namespaces but allows any other namespace; after you compute ns (and default to "ocp") and name (trimmed), add a check that rejects any namespace not in the allowed set {"ocp","ocp-private"} by appending to errs (use relPath, ns and name in the error message), while keeping the existing specific checks against release and releasePriv for "ocp" and "ocp-private" respectively; update the same block that sets ns/name so the new generic namespace validation runs before or alongside the existing release-specific validations.
182-185:⚠️ Potential issue | 🟠 MajorApply ignore short-circuit before both validation paths
Currently ignored repos are skipped for main/master checks (Line 183) but still processed in the release-branch block (Line 208+).
Suggested fix
orgRepo := fmt.Sprintf("%s/%s", info.Org, info.Repo) relPath := filepath.Join(info.Org, info.Repo, filepath.Base(info.Filename)) + if ignore.Has(orgRepo) { + return nil + } if isMainOrMasterBranch(info.Branch) { - if ignore.Has(orgRepo) { - return nil - } if cfg.PromotionConfiguration == nil { return nil }Also applies to: 208-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 182 - 185, The ignore check is only applied inside the isMainOrMasterBranch path so ignored repos still hit the release-branch validation; move/duplicate the ignore short-circuit so it runs before branching on info.Branch. Concretely, in the function that contains isMainOrMasterBranch(info.Branch) and the release-branch block (references: isMainOrMasterBranch(info.Branch), ignore.Has(orgRepo), info.Branch, orgRepo, and the release-branch validation block around lines 208-216), perform if ignore.Has(orgRepo) { return nil } immediately after obtaining orgRepo/info and before any branch-specific checks so both main/master and release-branch paths are skipped for ignored repos.
81-90:⚠️ Potential issue | 🟠 MajorPropagate
_prowconfig.yamlread/parse failures instead of silently skipping checksAt Line 83 and Line 88, errors are converted to
false, and Line 209 treats that as out-of-scope. This can suppress real violations when Prow config is broken.Suggested fix
-func hasCurrentReleaseBranchInProw(prowConfigDir, org, repo, currentRelease string) bool { +func hasCurrentReleaseBranchInProw(prowConfigDir, org, repo, currentRelease string) (bool, error) { path := filepath.Join(prowConfigDir, org, repo, "_prowconfig.yaml") data, err := os.ReadFile(path) if err != nil { - return false + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("read prow config %s: %w", path, err) } var cfg prowConfig if err := yaml.Unmarshal(data, &cfg); err != nil { - return false + return false, fmt.Errorf("unmarshal prow config %s: %w", path, err) } @@ if want.Has(strings.TrimSpace(b)) { - return true + return true, nil } } } - return false + return false, nil } @@ - inScopeProw := prowConfigDir != "" && hasCurrentReleaseBranchInProw(prowConfigDir, info.Org, info.Repo, release) + inScopeProw := false + if prowConfigDir != "" { + var err error + inScopeProw, err = hasCurrentReleaseBranchInProw(prowConfigDir, info.Org, info.Repo, release) + if err != nil { + return err + } + }Also applies to: 209-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/mainpromotion/validate.go` around lines 81 - 90, The function hasCurrentReleaseBranchInProw currently swallows file read and YAML unmarshal errors by returning false; change its signature to return (bool, error), propagate and return the underlying os.ReadFile and yaml.Unmarshal errors (e.g., return false, fmt.Errorf(...)) instead of false, and update all callers (the code that treats a false as "out-of-scope", referenced where hasCurrentReleaseBranchInProw is invoked around the validation flow) to check and handle the error separately so real Prow config read/parse failures surface as errors rather than being treated as a missing branch.
🤖 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/mainpromotion/validate.go`:
- Around line 193-203: The current validation only checks the "ocp" and
"ocp-private" namespaces but allows any other namespace; after you compute ns
(and default to "ocp") and name (trimmed), add a check that rejects any
namespace not in the allowed set {"ocp","ocp-private"} by appending to errs (use
relPath, ns and name in the error message), while keeping the existing specific
checks against release and releasePriv for "ocp" and "ocp-private" respectively;
update the same block that sets ns/name so the new generic namespace validation
runs before or alongside the existing release-specific validations.
- Around line 182-185: The ignore check is only applied inside the
isMainOrMasterBranch path so ignored repos still hit the release-branch
validation; move/duplicate the ignore short-circuit so it runs before branching
on info.Branch. Concretely, in the function that contains
isMainOrMasterBranch(info.Branch) and the release-branch block (references:
isMainOrMasterBranch(info.Branch), ignore.Has(orgRepo), info.Branch, orgRepo,
and the release-branch validation block around lines 208-216), perform if
ignore.Has(orgRepo) { return nil } immediately after obtaining orgRepo/info and
before any branch-specific checks so both main/master and release-branch paths
are skipped for ignored repos.
- Around line 81-90: The function hasCurrentReleaseBranchInProw currently
swallows file read and YAML unmarshal errors by returning false; change its
signature to return (bool, error), propagate and return the underlying
os.ReadFile and yaml.Unmarshal errors (e.g., return false, fmt.Errorf(...))
instead of false, and update all callers (the code that treats a false as
"out-of-scope", referenced where hasCurrentReleaseBranchInProw is invoked around
the validation flow) to check and handle the error separately so real Prow
config read/parse failures surface as errors rather than being treated as a
missing branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72ead936-ac21-4bbd-93a4-6fe0e38bcfbd
📒 Files selected for processing (3)
cmd/determinize-ci-operator/main.gopkg/mainpromotion/validate.gopkg/mainpromotion/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mainpromotion/validate_test.go
Contributor
Author
|
/test images e2e |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
danilo-gemoli
suggested changes
Mar 20, 2026
deepsm007
force-pushed
the
determinize-validate-main-promotion
branch
from
54cb6f0 to
d06bbd0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/validation/promotion/validate.go (1)
188-195: Avoid reparsing the full CI-operator config tree twice.
collectReposWithMainMaster()only usesconfig.Info, but it still does a fullOperateOnCIOperatorConfigDir()pass before the real validation pass. On a release-repo-sized config tree, that extra YAML walk is avoidable overhead for this presubmit path. A lightweight branch index from file paths, or buffering per-repo state during a single traversal, would keep this to one parse.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/promotion/validate.go` around lines 188 - 195, collectReposWithMainMaster currently triggers a full OperateOnCIOperatorConfigDir pass just to read config.Info and populate options.reposWithMainMaster, causing an unnecessary second YAML traversal; change the flow so we do not call OperateOnCIOperatorConfigDir from collectReposWithMainMaster — instead populate reposWithMainMaster during the existing single ci-operator traversal (or build a lightweight branch index from file paths) by updating the main validation traversal to insert fmt.Sprintf("%s/%s", info.Org, info.Repo) into options.reposWithMainMaster when isMainOrMasterBranch(info.Branch) is true; remove the extra traversal invocation in collectReposWithMainMaster and ensure any callers rely on the populated map from the single pass (adjust function signatures if needed).pkg/validation/promotion/validate_test.go (1)
353-409: Add a direct regression test for the Tide-gated release-branch rule.The suite covers malformed
_prowconfig.yaml, but it never exercises the main branch in Lines 259-273 ofpkg/validation/promotion/validate.go: same repo hasmain/master, Tide includes the current release, and therelease-X/openshift-Xconfig still has an enabled promotion target. That is the core enforcement path behind--prow-config-dir.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/promotion/validate_test.go` around lines 353 - 409, Update the test TestValidate_ErrorsOnMalformedProwConfigWhenEnforcingReleaseBranch to directly exercise the Tide-gated release-branch rule by creating a valid main branch config (foo-bar-main.yaml) and a release config (foo-bar-release-4.22.yaml) with an enabled promotion target, and write a _prowconfig.yaml into prowDir that defines a tide query including the current release (4.22) and the repo (foo/bar) so the code path in Validate (the Tide-gated check in validate.go lines around the main/master + tide + release target logic) runs; then call Validate(configDir, "4.22", prowDir, "", nil) and assert it returns an error. Ensure the test uses the same filenames and directories already referenced (_prowconfig.yaml, foo-bar-main.yaml, foo-bar-release-4.22.yaml) so the Validate function and its tide-based enforcement path are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/validation/promotion/validate_test.go`:
- Around line 353-409: Update the test
TestValidate_ErrorsOnMalformedProwConfigWhenEnforcingReleaseBranch to directly
exercise the Tide-gated release-branch rule by creating a valid main branch
config (foo-bar-main.yaml) and a release config (foo-bar-release-4.22.yaml) with
an enabled promotion target, and write a _prowconfig.yaml into prowDir that
defines a tide query including the current release (4.22) and the repo (foo/bar)
so the code path in Validate (the Tide-gated check in validate.go lines around
the main/master + tide + release target logic) runs; then call
Validate(configDir, "4.22", prowDir, "", nil) and assert it returns an error.
Ensure the test uses the same filenames and directories already referenced
(_prowconfig.yaml, foo-bar-main.yaml, foo-bar-release-4.22.yaml) so the Validate
function and its tide-based enforcement path are exercised.
In `@pkg/validation/promotion/validate.go`:
- Around line 188-195: collectReposWithMainMaster currently triggers a full
OperateOnCIOperatorConfigDir pass just to read config.Info and populate
options.reposWithMainMaster, causing an unnecessary second YAML traversal;
change the flow so we do not call OperateOnCIOperatorConfigDir from
collectReposWithMainMaster — instead populate reposWithMainMaster during the
existing single ci-operator traversal (or build a lightweight branch index from
file paths) by updating the main validation traversal to insert
fmt.Sprintf("%s/%s", info.Org, info.Repo) into options.reposWithMainMaster when
isMainOrMasterBranch(info.Branch) is true; remove the extra traversal invocation
in collectReposWithMainMaster and ensure any callers rely on the populated map
from the single pass (adjust function signatures if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2a70792-30b7-4061-82d6-a526763dd1f4
📒 Files selected for processing (3)
cmd/determinize-ci-operator/main.gopkg/validation/promotion/validate.gopkg/validation/promotion/validate_test.go
…r promotion validation Add pkg/validation/promotion with Validate() and tests. Wire determinize-ci-operator flags --validate-main-promotion, --current-release, --infra-periodics, --prow-config-dir, --main-promotion-ignore.
deepsm007
force-pushed
the
determinize-validate-main-promotion
branch
from
d06bbd0 to
7f38547
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/validation/promotion/validate.go (1)
126-129: Support both--current-releaseflag spellings when parsing infra-periodics.Line 128 only handles
--current-release=<value>. If the brancher job ever switches to the equally valid--current-release,<value>form, auto-discovery breaks even though the job still declares the release.Suggested fix
- for _, c := range job.Spec.Containers { - for _, arg := range c.Args { - if m := currentReleaseArgRE.FindStringSubmatch(strings.TrimSpace(arg)); len(m) > 1 { - return strings.TrimSpace(m[1]), nil - } - } - } + for _, c := range job.Spec.Containers { + for i, arg := range c.Args { + arg = strings.TrimSpace(arg) + if m := currentReleaseArgRE.FindStringSubmatch(arg); len(m) > 1 { + return strings.TrimSpace(m[1]), nil + } + if arg == "--current-release" && i+1 < len(c.Args) { + return strings.TrimSpace(c.Args[i+1]), nil + } + } + }A split-arg case in
TestParseCurrentReleaseFromInfraPeriodicsDatawould lock this in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/promotion/validate.go` around lines 126 - 129, The parser only supports the combined form `--current-release=<value>` via currentReleaseArgRE inside the loop over job.Spec.Containers/Args; update the loop to also handle the split-argument form by checking if arg == "--current-release" and, if so, returning the next argument (trimmed) when present. Keep the existing regex match (currentReleaseArgRE) for the combined form, and add the split-arg branch that safely checks bounds on the Args slice before returning. Add a unit case to TestParseCurrentReleaseFromInfraPeriodicsData to assert both `--current-release=<value>` and `--current-release`, `<value>` are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/validation/promotion/validate.go`:
- Around line 126-129: The parser only supports the combined form
`--current-release=<value>` via currentReleaseArgRE inside the loop over
job.Spec.Containers/Args; update the loop to also handle the split-argument form
by checking if arg == "--current-release" and, if so, returning the next
argument (trimmed) when present. Keep the existing regex match
(currentReleaseArgRE) for the combined form, and add the split-arg branch that
safely checks bounds on the Args slice before returning. Add a unit case to
TestParseCurrentReleaseFromInfraPeriodicsData to assert both
`--current-release=<value>` and `--current-release`, `<value>` are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09f3f969-bbc7-4a0f-899a-b76f54a90e8f
📒 Files selected for processing (3)
cmd/determinize-ci-operator/main.gopkg/validation/promotion/validate.gopkg/validation/promotion/validate_test.go
Contributor
Author
|
/retest |
Contributor
|
@deepsm007: The following test 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. |
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
3 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.
What
Add main-promotion validation to
determinize-ci-operatorso the same rules enforced in openshift/release (via a standalone script) run inside the existing toolchain. No new script in release once release repo is updated to call determinize with the new flag.How
pkg/mainpromotion:Validate()enforces (1) main/master configs promote only to the current release (ocp/{current},ocp-private/{current}-priv), and (2) release-X/openshift-X configs have promotion disabled when the repo has main/master and Prow has that release in a tide query’sincludedBranches. Uses the same ignore list as the release script (gatekeeper, offline_migration, etc.; cri-o is not ignored).determinize-ci-operator:--validate-main-promotion,--current-release,--prow-config-dir,--infra-periodics,--main-promotion-ignore. When--validate-main-promotionis set, validation runs first and the process exits 1 on violations. Current release can come from--current-releaseor from theperiodic-prow-auto-config-brancherjob in--infra-periodics.Release repo follow-up
After this merges, release will switch
check-validate-main-promotionto run determinize-ci-operator with--validate-main-promotionand the appropriate mounts (--config-dir,--infra-periodics,--prow-config-dir) and removehack/validate-main-promotion-guard.py./cc @danilo-gemoli