ROSAENG-62441: Skip SAPM-managed targets and add --latest flag to promote rhobs#937
ROSAENG-62441: Skip SAPM-managed targets and add --latest flag to promote rhobs#937dustman9000 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dustman9000: This pull request references ROSAENG-62441 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughRHOBS promotion now includes ChangesRHOBS promotion workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RHOBSCommand
participant ConfigurationGit
participant BulkPromotion
participant GitCommit
User->>RHOBSCommand: invoke --latest
RHOBSCommand->>ConfigurationGit: fetch main and resolve pinned SHA
ConfigurationGit-->>RHOBSCommand: return validated SHA
RHOBSCommand->>BulkPromotion: promote services with SHA
BulkPromotion->>ConfigurationGit: read old-to-new git history
ConfigurationGit-->>BulkPromotion: return changelog
BulkPromotion->>GitCommit: create one consolidated promotion commit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000 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
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 111-123: Update the filtering loop in the promotion target flow to
capture and propagate the error returned by kyaml.Lookup("promotion",
"subscribe").Filter(t), returning nil and that error immediately when the lookup
fails; only treat a successful nil subscribeNode as “not subscribed” and append
the target.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7ff544c-9e1b-4bd4-9349-936eb44a8837
📒 Files selected for processing (1)
cmd/promote/rhobs/rhobs.go
2548fd0 to
9521649
Compare
dustman9000
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 151-180: Update findUpstreamRemote and resolveLatestHash to accept
or derive a context with a bounded timeout, and replace each git exec.Command
invocation with exec.CommandContext using that context. Ensure cancellation and
timeout errors are propagated through the existing error paths, while preserving
the current remote discovery, fetch, and hash resolution behavior.
- Line 180: Handle and propagate the error returned by fmt.Printf at
cmd/promote/rhobs/rhobs.go lines 180 and 343, aborting the current flow when
stdout writes fail before any promotion side effects occur. Update the
surrounding functions to return the write error through their existing error
paths.
- Around line 169-176: The git fetch and revision-resolution flow must use the
ref produced by the current fetch rather than a potentially stale
remote-tracking ref. Update the commands around the fetch in the promotion logic
to explicitly fetch into refs/remotes/<remote>/main or resolve FETCH_HEAD, and
add coverage for a remote whose fetch refspec does not map main.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2673b0e3-659e-4a7c-864c-86a8e223e27e
📒 Files selected for processing (1)
cmd/promote/rhobs/rhobs.go
9521649 to
a4ed3e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 365-368: Update the list-mode incompatibility check in the command
flow around ops.latest so --list rejects an explicitly supplied --latest before
returning. Include ops.latest alongside the existing incompatible options, while
preserving the current validation for non-list promotion operations.
- Around line 223-229: Update the changelog generation around changeLog before
any app-interface worktree mutations: use a context-aware, bounded git command,
validate required inputs and propagate failures instead of silently leaving
changeLog empty, wrapping the command error with promotion context. Ensure
promotion aborts before target mutations when changelog generation fails, while
preserving the existing trimmed git log output on success.
- Around line 156-158: Update the remote-selection loop that checks
“rhobs/configuration” and “(fetch)” to normalize both SSH and HTTPS remote URL
forms, then allow only the expected GitLab host and exact repository path.
Return the remote only after it passes this host/path allow-list, rather than
matching arbitrary URLs containing the repository text.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b7f6dd34-ce4a-46c0-a7d9-c8cdeb1a9fc1
📒 Files selected for processing (2)
cmd/promote/rhobs/rhobs.gopkg/promote/service.go
a4ed3e2 to
086a69c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/promote/rhobs/rhobs.go (3)
112-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate the
kyaml.Filterlookup error.
kyaml.Lookup("promotion", "subscribe").Filter(t)'s error is discarded (_). A genuine traversal error (not "path missing") will silently fall through as "not subscribed" and the target may get incorrectly filtered in or out. Return the error instead of ignoring it.Previously flagged on an earlier revision of this same code and not yet addressed.
As per path instructions,
**/*.gofiles should "Never ignore error returns."🔧 Proposed fix
- subscribeNode, _ := kyaml.Lookup("promotion", "subscribe").Filter(t) + subscribeNode, err := kyaml.Lookup("promotion", "subscribe").Filter(t) + if err != nil { + return nil, err + } if subscribeNode != nil { continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promote/rhobs/rhobs.go` around lines 112 - 126, Update rhobsPromoteCallbacks.FilterTargets to capture and propagate the error returned by kyaml.Lookup("promotion", "subscribe").Filter(t) instead of discarding it; retain the existing nil-node filtering behavior only when the lookup succeeds.Source: Path instructions
151-192: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the git subprocess calls with a context/timeout.
findUpstreamRemote/resolveLatestHashinvokeexec.Command(lines 157, 179, 182) for a network-touchinggit fetch, with no cancellation or timeout — a hung remote can block the CLI indefinitely. Useexec.CommandContextwith a bounded timeout derived fromcmd.Context().Previously flagged on an earlier revision and not yet addressed.
Note: the ast-grep CWE-78 hints on the
remote+"/main"argument are false positives here —exec.Commandtakes each argument as a discrete argv element (no shell is invoked), so string concatenation into a single arg does not enable command injection.As per path instructions,
**/*.gofiles should use "context.Context for cancellation and timeouts."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promote/rhobs/rhobs.go` around lines 151 - 192, Update findUpstreamRemote and resolveLatestHash to accept a context.Context and use exec.CommandContext for every git subprocess, including remote listing, fetch, and rev-parse. Derive a bounded timeout from the caller’s cmd.Context(), propagate cancellation and timeout errors, and update all callers to pass that context while preserving the existing remote-resolution behavior.Source: Path instructions
190-190: 🩺 Stability & Availability | 🟡 Minor | 💤 Low valueUnchecked
fmt.Printf/exec output errors throughout the promotion flow.All of these
fmt.Printfcalls (progress/status output) ignore the returned error, includingchangeLoggeneration viaexec.Command(...).Output()at line 211 for the injection hint noted above (false positive, same reasoning as the fetch/rev-parse calls). This mirrors a previously flagged, not-yet-addressed concern that a closed stdout pipe should abort before promotion side effects run rather than being silently swallowed; the same pattern now also appears at the new--latestprint sites (373, 388).As per path instructions, "Never ignore error returns."
Also applies to: 211-211, 227-227, 235-235, 252-253, 373-373, 388-388
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promote/rhobs/rhobs.go` at line 190, Handle every error returned by the promotion flow’s fmt.Printf calls, including the status sites around fetch and the --latest output, and propagate or return failures before continuing. Also check the error from exec.Command(...).Output() used for changeLog generation, preserving its output only when successful. Ensure closed stdout or command failures abort the flow before promotion side effects occur.Source: Path instructions
🧹 Nitpick comments (1)
cmd/promote/rhobs/rhobs.go (1)
151-172: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
configRepoURLPatternsmatching isn't boundary-anchored.
strings.Contains(line, pattern)matches any URL containinggitlab.cee.redhat.com/rhobs/configurationas a prefix of a longer path (e.g..../rhobs/configuration-mirror), which would still be treated as the canonical repo. Anchor the match on a path/URL boundary (e.g. require a trailing/,.git, or end-of-string) rather than a bare substring check.As per path instructions, "Validate at trust boundaries with allow-lists, not deny-lists." This narrows an already-improved allow-list rather than introducing a new gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/promote/rhobs/rhobs.go` around lines 151 - 172, Update findUpstreamRemote’s configRepoURLPatterns matching so each allowed repository pattern must end at a valid URL boundary: trailing slash, .git, or end-of-string. Replace the bare strings.Contains check with boundary-aware allow-list matching, while preserving fetch-only filtering and returning the matched remote unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 242-246: Update the commit message construction around ChangesURL
to detect an empty oldHash before building the GitLab compare link. Preserve the
compare URL when oldHash is non-empty, and use an appropriate non-malformed
fallback when getCurrentProductionHash returns empty so the commit message
remains valid.
---
Duplicate comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 112-126: Update rhobsPromoteCallbacks.FilterTargets to capture and
propagate the error returned by kyaml.Lookup("promotion", "subscribe").Filter(t)
instead of discarding it; retain the existing nil-node filtering behavior only
when the lookup succeeds.
- Around line 151-192: Update findUpstreamRemote and resolveLatestHash to accept
a context.Context and use exec.CommandContext for every git subprocess,
including remote listing, fetch, and rev-parse. Derive a bounded timeout from
the caller’s cmd.Context(), propagate cancellation and timeout errors, and
update all callers to pass that context while preserving the existing
remote-resolution behavior.
- Line 190: Handle every error returned by the promotion flow’s fmt.Printf
calls, including the status sites around fetch and the --latest output, and
propagate or return failures before continuing. Also check the error from
exec.Command(...).Output() used for changeLog generation, preserving its output
only when successful. Ensure closed stdout or command failures abort the flow
before promotion side effects occur.
---
Nitpick comments:
In `@cmd/promote/rhobs/rhobs.go`:
- Around line 151-172: Update findUpstreamRemote’s configRepoURLPatterns
matching so each allowed repository pattern must end at a valid URL boundary:
trailing slash, .git, or end-of-string. Replace the bare strings.Contains check
with boundary-aware allow-list matching, while preserving fetch-only filtering
and returning the matched remote unchanged.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0cf88a8-043f-46c8-a792-c2749640dfe1
📒 Files selected for processing (4)
cmd/promote/rhobs/rhobs.godocs/README.mddocs/osdctl_promote_rhobs.mdpkg/promote/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/promote/service.go
086a69c to
85cc465
Compare
85cc465 to
f456c6c
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
…mote rhobs - FilterTargets now excludes production targets that have a promotion.subscribe field, since those are auto-promoted by SAPM wave gating and should not be manually pinned. - Bulk promote skips saas files that don't reference rhobs/configuration (e.g. synthetics-agent, synthetics-api). - Add saas-mc-rules to the SRE-owned services list. - Add --latest flag that fetches and resolves main from the local rhobs/configuration checkout, auto-detecting the upstream remote by URL (restricted to gitlab.cee.redhat.com/rhobs/configuration). - Bulk promote now produces a single commit with compare link and commit log, matching the single-service path format.
f456c6c to
964cf7d
Compare
|
@dustman9000: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
FilterTargetsnow excludes production targets that have apromotion.subscribefield, since those are auto-promoted by SAPM wave gating and should not be manually pinnedsaas-mc-rulesto the SRE-owned services list--latestflag that auto-resolvesorigin/mainfrom the local rhobs/configuration checkout, detecting the upstream remote by URLUsage
Context
RHOBS rules saas files now use SAPM wave gating: wave-1 is manually promoted, wave-2/3 auto-promote after post-deploy checks pass and soakDays complete.
osdctl promote rhobsneeds to only pin wave-1 targets.The bulk path (
promoteAllServices) already skips targets withsubscribe:viatargetBlockHasSubscribe(). This PR fixes the single-service path (--serviceId) to do the same via the kyamlLookupAPI.Jira: ROSAENG-62441
Test plan
go build ./...passesgo test ./cmd/promote/rhobs/ ./pkg/promote/passesosdctl promote rhobs --latestresolves SHA and promotes wave-1 targets onlyosdctl promote rhobs --serviceId saas-hcp-rules --gitHash <sha>only pins wave-1 targetosdctl promote rhobs --latest --gitHash <sha>errors (mutually exclusive)Summary by CodeRabbit
--latestpromotion mode to bulk-promote all RHOBS services to the latestrhobs/configurationorigin/mainpinned SHA.saas-mc-rules.rhobs-productionand skipping targets that already include apromotion.subscribenode.osdctl promote rhobsdocs with the new--latestworkflow, examples, and option details.