CNTRLPLANE-3773: Rewrite find-push-pipelinerun as Go CLI - #8935
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@celebdor: This pull request references CNTRLPLANE-3773 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 task 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. |
celebdor
added
the
area/ci-tooling
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds a new Sequence Diagram(s)sequenceDiagram
participant User
participant Main as main.go
participant PR as pr.go
participant Query as query.go
participant Watch as watch.go
participant GitHub as GitHub API
participant Kube as Kubernetes APIs
User->>Main: run tool with PR input
Main->>PR: ResolvePR()
PR->>GitHub: GetMergeSHA()
Main->>Query: ListPipelineRuns(sha)
Query->>Kube: GET PipelineRuns
Main->>Watch: WatchBuildPipeline() / WatchReleasePipeline()
Watch->>Query: list PipelineRuns / Releases
Query->>Kube: GET resources
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: celebdor The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
area/documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8935 +/- ##
==========================================
+ Coverage 43.34% 43.45% +0.11%
==========================================
Files 771 771
Lines 95534 95718 +184
==========================================
+ Hits 41408 41597 +189
+ Misses 51242 51234 -8
- Partials 2884 2887 +3 see 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 (3)
hack/tools/scripts/find-push-pipelinerun_test.bats (1)
229-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSynthetic table bypasses
format_releasesoutput contract.These tests hand-craft the input table with
printfinstead of piping realformat_releasesoutput intohas_pending_releases. This tests the awk$4parsing logic in isolation but won't catch a regression ifformat_releases' column order/count changes (e.g. STATUS moves from position 4). Consider at least one test that chainsformat_releases | has_pending_releasesfor end-to-end contract coverage.🤖 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 `@hack/tools/scripts/find-push-pipelinerun_test.bats` around lines 229 - 245, The current `has_pending_releases` tests only validate `awk` parsing against a hand-built table, so they can miss regressions in the `format_releases` output contract. Add at least one end-to-end test that feeds real `format_releases` output directly into `has_pending_releases`, using the existing `format_releases` and `has_pending_releases` helpers, so the STATUS column position and column count are verified together.docs/content/contribute/konflux-scripts.md (1)
149-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid over-specifying the release PipelineRun name.
The supplied implementation context matches Release PipelineRuns by label, not by a
managed-*name pattern. Please verify that naming is guaranteed; otherwise rephrase this bullet to describe the label-based match instead.Details
Suggested wording
-- **Release PipelineRuns** — the `managed-*` PipelineRuns in the - `rhtap-releng-tenant` namespace that execute the release (push to - registry, sign, create advisory, etc.). +- **Release PipelineRuns** — the PipelineRuns in the + `rhtap-releng-tenant` namespace associated with the Release label + that execute the release (push to registry, sign, create advisory, etc.).🤖 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 `@docs/content/contribute/konflux-scripts.md` around lines 149 - 155, The “Release PipelineRuns” bullet over-specifies a managed-* name pattern that is not guaranteed by the implementation. Update the documentation in the contribute/konflux-scripts content to describe how these PipelineRuns are actually identified by labels rather than by name, using the Release PipelineRuns wording to match the existing release flow context.hack/tools/scripts/find-push-pipelinerun.sh (1)
335-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
snapshot_jsondeclaration.
snapshot_jsonis declared but never assigned or read (the digest is derived directly intosource_digest).🧹 Drop the dead local
- local rel_name snapshot_json source_digest + local rel_name source_digest🤖 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 `@hack/tools/scripts/find-push-pipelinerun.sh` at line 335, Remove the dead local declaration in the shell script: the `snapshot_json` variable in `find-push-pipelinerun.sh` is never used because the digest is already handled through `source_digest`. Update the local variable declaration near the related parsing logic to keep only the identifiers that are actually assigned and read, and ensure the surrounding flow in the same script still uses `rel_name` and `source_digest` as before.Source: Linters/SAST tools
🤖 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 `@hack/tools/scripts/find-push-pipelinerun.sh`:
- Around line 512-527: The watch loop in find-push-pipelinerun.sh can exit too
early after PipelineRuns finish because the Release CR may not exist yet; in the
watch block around query_releases and has_pending_releases, avoid breaking when
query_releases returns no results or fails immediately after push completion.
Update the logic in the watch loop so it keeps polling until a Release appears
and its release pipeline is no longer pending, using the existing helpers
query_releases, format_releases, has_pending, and has_pending_releases to locate
the behavior.
- Around line 278-291: The STATUS field in the jq formatting block is
order-dependent because it reads .status.conditions[0].reason, which can pick
the wrong condition. Update the transformation in the find-push-pipelinerun.sh
formatting pipeline to explicitly select the Released condition by name, and use
that condition’s reason for the STATUS column so the output reflects the release
outcome. Keep the change within the jq expression that builds the items list and
sorts by created.
---
Nitpick comments:
In `@docs/content/contribute/konflux-scripts.md`:
- Around line 149-155: The “Release PipelineRuns” bullet over-specifies a
managed-* name pattern that is not guaranteed by the implementation. Update the
documentation in the contribute/konflux-scripts content to describe how these
PipelineRuns are actually identified by labels rather than by name, using the
Release PipelineRuns wording to match the existing release flow context.
In `@hack/tools/scripts/find-push-pipelinerun_test.bats`:
- Around line 229-245: The current `has_pending_releases` tests only validate
`awk` parsing against a hand-built table, so they can miss regressions in the
`format_releases` output contract. Add at least one end-to-end test that feeds
real `format_releases` output directly into `has_pending_releases`, using the
existing `format_releases` and `has_pending_releases` helpers, so the STATUS
column position and column count are verified together.
In `@hack/tools/scripts/find-push-pipelinerun.sh`:
- Line 335: Remove the dead local declaration in the shell script: the
`snapshot_json` variable in `find-push-pipelinerun.sh` is never used because the
digest is already handled through `source_digest`. Update the local variable
declaration near the related parsing logic to keep only the identifiers that are
actually assigned and read, and ensure the surrounding flow in the same script
still uses `rel_name` and `source_digest` as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 203b1bf3-2a96-4ada-a115-549bcd139464
📒 Files selected for processing (3)
docs/content/contribute/konflux-scripts.mdhack/tools/scripts/find-push-pipelinerun.shhack/tools/scripts/find-push-pipelinerun_test.bats
Replace the 535-line bash script with a Go CLI tool that uses client-go for Kubernetes auth and the GitHub REST API directly. Key improvements over the bash version: - Interface-based Querier enables mock-driven tests for the watch loop, which was untestable in bash - Release status selects the "Released" condition by type instead of assuming conditions[0] - Watch loop continues polling when no Release CR exists yet (fixes premature exit after builds complete) - KubeArchive fallback messages are deduplicated - Release PipelineRuns are consolidated into a single table - No dependency on jq, curl, or gh CLI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bed58a4 to
fd53cb3
Compare
celebdor
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
hack/tools/find-push-pipelinerun/query.go (2)
100-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOriginal live-query error is discarded when KubeArchive fallback is attempted.
When
listResourceagainst the live cluster fails with a real error (auth, RBAC, network) rather than just returning zero items,listWithFallbackproceeds silently to KubeArchive without logging the original error. If the archive query also fails, only the archive error is surfaced, hiding the root cause of the live failure.🔎 Proposed fix
items, err := listResource[T](q, q.kubeHost, apiPath, labelSelector) if err == nil && len(items) > 0 { return items, nil } + if err != nil { + fmt.Fprintf(q.stderr, "Live query for %s failed: %v; falling back to KubeArchive...\n", resourceName, err) + }🤖 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 `@hack/tools/find-push-pipelinerun/query.go` around lines 100 - 125, The live-cluster error in listWithFallback is being dropped when the KubeArchive fallback runs, so preserve and report the original listResource failure from q.kubeHost before querying q.kaHost. Update listWithFallback to log or wrap the live-query error when err is non-nil and items is empty, and if the archive lookup also fails, include both the original live error and the KubeArchive error in the returned error so the root cause is not hidden.
127-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate request/response handling in
listResourceandgetResource.Both functions repeat identical URL parsing, request creation, status-code checking, and error-body reading. Extracting a shared
doRequest(q, host, path) (*http.Response, error)helper would remove the duplication.♻️ Sketch of a shared helper
+func (q *httpQuerier) doRequest(host, path string) (*http.Response, error) { + u, err := url.Parse(host) + if err != nil { + return nil, fmt.Errorf("parsing host URL %q: %w", host, err) + } + u.Path = path + req, err := http.NewRequest("GET", u.String(), nil) //nolint:noctx + if err != nil { + return nil, err + } + resp, err := q.client.Do(req) + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + return nil, fmt.Errorf("HTTP %d from %s: %s", resp.StatusCode, u.String(), string(body)) + } + return resp, nil +}Also applies to: 160-188
🤖 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 `@hack/tools/find-push-pipelinerun/query.go` around lines 127 - 158, The request/response setup in listResource and getResource is duplicated, including URL parsing, GET request creation, status checking, and error-body handling. Extract that shared logic into a reusable helper such as doRequest on the httpQuerier path, then have both listResource and getResource call it and keep only their resource-specific decode logic. Make sure the helper preserves the existing error formatting and response body handling so both call sites behave the same.hack/tools/find-push-pipelinerun/watch.go (1)
54-54: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
ListReleasescall per iteration, and its errors are silently swallowed forever.
printReleasePipeline(line 76) and the terminal-state check (line 60) both callq.ListReleases(cfg.SHA)independently each iteration — doubling API calls for no benefit (confirmed by the comment inwatch_test.go'sTestWatchReleasePipeline_ExitsWhenAllTerminal). Additionally, line 61 treats anyListReleaseserror the same as "release not yet created" and silentlycontinues — a persistent RBAC/network error would loop forever with no diagnostic beyond "--- refreshing ---", unlikeListPipelineRunserrors which are returned immediately.♻️ Proposed consolidation
- printReleasePipeline(q, cfg.SHA, cfg.Component, cfg.Stdout, cfg.Stderr) - if HasPending(prs) { continue } - releases, err := q.ListReleases(cfg.SHA) - if err != nil || len(releases) == 0 { - // No releases yet — keep polling + if err != nil { + fmt.Fprintf(cfg.Stderr, "Warning: failed to query Releases: %v\n", err) + continue + } + if len(releases) == 0 { continue } if cfg.Component != "" { releases = FilterReleasesByComponent(releases, cfg.Component) } + printReleasePipelineWithReleases(q, releases, cfg.Stdout, cfg.Stderr) if !HasPendingReleases(releases) { return nil }Also applies to: 60-70, 75-79
🤖 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 `@hack/tools/find-push-pipelinerun/watch.go` at line 54, `watch` is making two `ListReleases` calls per loop and swallowing release-listing errors, so consolidate the release lookup in the main iteration and reuse it for both the terminal-state check and `printReleasePipeline`. Update the `watch` loop and `printReleasePipeline` to accept the already-fetched releases (or otherwise avoid re-calling `q.ListReleases(cfg.SHA)`), and change the `ListReleases` error handling to surface persistent failures instead of silently continuing, similar to how `ListPipelineRuns` errors are handled.hack/tools/find-push-pipelinerun/format.go (2)
169-243: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant ReleasePlan/ReleasePlanAdmission lookups across releases.
ResolveDestImagescallsresolveDestImage→getMappingForReleaseonce per release, re-fetching the sameReleasePlan(and possiblyReleasePlanAdmission) via HTTP every time, even when many releases share the same plan (typical for a single push build with multiple component releases). Caching by plan name withinResolveDestImageswould cut this down to one request per distinct plan.♻️ Proposed fix
func ResolveDestImages(q Querier, releases []Release) map[string]string { images := make(map[string]string) + mappingCache := make(map[string]*Mapping) for _, rel := range releases { - img := resolveDestImage(q, rel) + img := resolveDestImage(q, rel, mappingCache) if img != "" { images[rel.Metadata.Name] = img } } return images } -func resolveDestImage(q Querier, rel Release) string { - mapping := getMappingForRelease(q, rel.Spec.ReleasePlan) +func resolveDestImage(q Querier, rel Release, cache map[string]*Mapping) string { + mapping, ok := cache[rel.Spec.ReleasePlan] + if !ok { + mapping = getMappingForRelease(q, rel.Spec.ReleasePlan) + cache[rel.Spec.ReleasePlan] = mapping + } if mapping == nil { return "" }🤖 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 `@hack/tools/find-push-pipelinerun/format.go` around lines 169 - 243, ResolveDestImages is repeatedly re-fetching the same ReleasePlan and ReleasePlanAdmission for releases that share a plan, causing redundant HTTP calls. Add plan-scoped caching inside ResolveDestImages so resolveDestImage/getMappingForRelease only loads each distinct release plan once per invocation, reusing the cached Mapping for subsequent releases with the same rel.Spec.ReleasePlan.
67-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated sort+tabwriter boilerplate across four Format functions.*
FormatPipelineRuns,FormatReleases,FormatReleasesWithImages, andFormatReleasePipelineRunseach repeat: empty check →sort.SlicebyCreationTimestamp→ build atabwriter→ print header/rows →Flush. Consider extracting the shared scaffolding (empty-check, sort, tabwriter setup/flush) into a small generic helper that each caller feeds with its header string and per-row column values, reducing duplication.🤖 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 `@hack/tools/find-push-pipelinerun/format.go` around lines 67 - 165, The four Format* functions repeat the same empty check, CreationTimestamp sort, tabwriter setup/flush, and row printing logic. Refactor the shared scaffolding in FormatPipelineRuns, FormatReleases, FormatReleasesWithImages, and FormatReleasePipelineRuns into a common helper that accepts the header and row-rendering callback, while keeping each function responsible only for its specific columns and data extraction.
🤖 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 `@hack/tools/find-push-pipelinerun/format.go`:
- Around line 13-40: The PipelineRun terminal status set in
pipelineRunTerminalStatuses is incomplete, so HasPending treats some finished
runs as pending and can cause WatchBuildPipeline and the PipelineRun path in
WatchReleasePipeline to poll forever. Update the terminal status map to include
any additional terminal PipelineRun reasons used by pipelineRunStatus, and keep
HasPending relying on that expanded set so finished runs are no longer reported
as pending.
In `@hack/tools/find-push-pipelinerun/main.go`:
- Around line 134-141: The watch interval parsing in watchInterval currently
accepts zero or negative values from WATCH_INTERVAL, which can make the polling
loop spin without delay. Update the watchInterval function to validate the
parsed integer and only return a custom duration when it is greater than zero;
otherwise fall back to defaultWatchInterval. Keep the change localized to
watchInterval and preserve the existing strconv.Atoi and time.Duration
conversion path for valid positive values.
In `@hack/tools/find-push-pipelinerun/pr.go`:
- Around line 76-133: The GitHub client created in newGitHubClient has no
request timeout, so a stalled API call can hang the tool indefinitely. Update
newGitHubClient to return an http.Client with a non-zero Timeout in both the
token and no-token cases, while keeping tokenTransport as the wrapper when
GITHUB_TOKEN is provided. Ensure GetMergeSHA continues to use the client
returned by newGitHubClient so all GitHub requests are covered.
In `@hack/tools/find-push-pipelinerun/query.go`:
- Around line 41-65: The HTTP querier currently has no cancellation or timeout
control, so requests in the watch loop can hang indefinitely. Update
newHTTPQuerier to configure a client-wide timeout on restConfig before calling
rest.HTTPClientFor, and ensure the request paths in httpQuerier use
context-aware request creation instead of nil contexts. Use the existing
newHTTPQuerier and httpQuerier symbols to centralize the timeout/cancellation
behavior so all external calls can recover if the API server or KubeArchive
endpoint stalls.
In `@hack/tools/find-push-pipelinerun/watch.go`:
- Around line 40-58: WatchReleasePipeline is fetching release data twice and
hiding real API failures by treating all ListReleases errors like “not found.”
Update WatchReleasePipeline and the printReleasePipeline path so releases are
only fetched once per loop, and distinguish expected empty/not-created cases
from unexpected errors. Use the existing Querier/ListReleases and
printReleasePipeline symbols to return non-expected errors immediately while
continuing to poll only for the missing-release case.
---
Nitpick comments:
In `@hack/tools/find-push-pipelinerun/format.go`:
- Around line 169-243: ResolveDestImages is repeatedly re-fetching the same
ReleasePlan and ReleasePlanAdmission for releases that share a plan, causing
redundant HTTP calls. Add plan-scoped caching inside ResolveDestImages so
resolveDestImage/getMappingForRelease only loads each distinct release plan once
per invocation, reusing the cached Mapping for subsequent releases with the same
rel.Spec.ReleasePlan.
- Around line 67-165: The four Format* functions repeat the same empty check,
CreationTimestamp sort, tabwriter setup/flush, and row printing logic. Refactor
the shared scaffolding in FormatPipelineRuns, FormatReleases,
FormatReleasesWithImages, and FormatReleasePipelineRuns into a common helper
that accepts the header and row-rendering callback, while keeping each function
responsible only for its specific columns and data extraction.
In `@hack/tools/find-push-pipelinerun/query.go`:
- Around line 100-125: The live-cluster error in listWithFallback is being
dropped when the KubeArchive fallback runs, so preserve and report the original
listResource failure from q.kubeHost before querying q.kaHost. Update
listWithFallback to log or wrap the live-query error when err is non-nil and
items is empty, and if the archive lookup also fails, include both the original
live error and the KubeArchive error in the returned error so the root cause is
not hidden.
- Around line 127-158: The request/response setup in listResource and
getResource is duplicated, including URL parsing, GET request creation, status
checking, and error-body handling. Extract that shared logic into a reusable
helper such as doRequest on the httpQuerier path, then have both listResource
and getResource call it and keep only their resource-specific decode logic. Make
sure the helper preserves the existing error formatting and response body
handling so both call sites behave the same.
In `@hack/tools/find-push-pipelinerun/watch.go`:
- Line 54: `watch` is making two `ListReleases` calls per loop and swallowing
release-listing errors, so consolidate the release lookup in the main iteration
and reuse it for both the terminal-state check and `printReleasePipeline`.
Update the `watch` loop and `printReleasePipeline` to accept the already-fetched
releases (or otherwise avoid re-calling `q.ListReleases(cfg.SHA)`), and change
the `ListReleases` error handling to surface persistent failures instead of
silently continuing, similar to how `ListPipelineRuns` errors are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d090f0f3-6dd8-41e6-b5e6-01cf0da12886
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (14)
Makefiledocs/content/contribute/konflux-scripts.mdhack/tools/find-push-pipelinerun/format.gohack/tools/find-push-pipelinerun/format_test.gohack/tools/find-push-pipelinerun/main.gohack/tools/find-push-pipelinerun/pr.gohack/tools/find-push-pipelinerun/pr_test.gohack/tools/find-push-pipelinerun/query.gohack/tools/find-push-pipelinerun/query_test.gohack/tools/find-push-pipelinerun/types.gohack/tools/find-push-pipelinerun/watch.gohack/tools/find-push-pipelinerun/watch_test.gohack/tools/scripts/find-push-pipelinerun.shhack/tools/scripts/find-push-pipelinerun_test.bats
💤 Files with no reviewable changes (2)
- hack/tools/scripts/find-push-pipelinerun.sh
- hack/tools/scripts/find-push-pipelinerun_test.bats
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/content/contribute/konflux-scripts.md
| var ( | ||
| pipelineRunTerminalStatuses = map[string]bool{ | ||
| "Completed": true, "Failed": true, "Succeeded": true, "Error": true, | ||
| } | ||
| releaseTerminalStatuses = map[string]bool{ | ||
| "Succeeded": true, "Failed": true, "Error": true, "Rejected": true, | ||
| } | ||
| ) | ||
|
|
||
| // HasPending returns true if any PipelineRun has a non-terminal status. | ||
| func HasPending(prs []PipelineRun) bool { | ||
| for _, pr := range prs { | ||
| if !pipelineRunTerminalStatuses[pipelineRunStatus(pr)] { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // HasPendingReleases returns true if any Release has a non-terminal status. | ||
| func HasPendingReleases(releases []Release) bool { | ||
| for _, rel := range releases { | ||
| if !releaseTerminalStatuses[releaseStatus(rel)] { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== format.go =="
nl -ba hack/tools/find-push-pipelinerun/format.go | sed -n '1,120p'
echo
echo "== watch loop usages =="
rg -n "HasPending\(|HasPendingReleases\(|WatchBuildPipeline|WatchReleasePipeline|pipelineRunTerminalStatuses|releaseTerminalStatuses" hack/tools/find-push-pipelinerun -n
echo
echo "== surrounding watch.go =="
nl -ba hack/tools/find-push-pipelinerun/watch.go | sed -n '1,220p'Repository: openshift/hypershift
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== format.go =="
cat -n hack/tools/find-push-pipelinerun/format.go | sed -n '1,120p'
echo
echo "== watch loop usages =="
rg -n "HasPending\\(|HasPendingReleases\\(|WatchBuildPipeline|WatchReleasePipeline|pipelineRunTerminalStatuses|releaseTerminalStatuses" hack/tools/find-push-pipelinerun
echo
echo "== watch.go =="
cat -n hack/tools/find-push-pipelinerun/watch.go | sed -n '1,240p'Repository: openshift/hypershift
Length of output: 10767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== status helpers =="
rg -n "func pipelineRunStatus|func releaseStatus|Succeeded|Reason|Condition" hack/tools/find-push-pipelinerun/*.go
echo
echo "== relevant slices of format.go =="
sed -n '120,260p' hack/tools/find-push-pipelinerun/format.go | cat -n
echo
echo "== format tests around HasPending =="
sed -n '100,290p' hack/tools/find-push-pipelinerun/format_test.go | cat -nRepository: openshift/hypershift
Length of output: 16571
Expand the PipelineRun terminal set. Any terminal reason missing from pipelineRunTerminalStatuses is treated as pending, so WatchBuildPipeline and the PipelineRun phase of WatchReleasePipeline can keep polling forever.
🤖 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 `@hack/tools/find-push-pipelinerun/format.go` around lines 13 - 40, The
PipelineRun terminal status set in pipelineRunTerminalStatuses is incomplete, so
HasPending treats some finished runs as pending and can cause WatchBuildPipeline
and the PipelineRun path in WatchReleasePipeline to poll forever. Update the
terminal status map to include any additional terminal PipelineRun reasons used
by pipelineRunStatus, and keep HasPending relying on that expanded set so
finished runs are no longer reported as pending.
| func WatchReleasePipeline(q Querier, cfg WatchConfig) error { | ||
| for { | ||
| fmt.Fprintf(cfg.Stderr, "\n--- refreshing (%s) ---\n\n", cfg.Interval) | ||
| time.Sleep(cfg.Interval) | ||
|
|
||
| prs, err := q.ListPipelineRuns(cfg.SHA) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if cfg.Component != "" { | ||
| prs = FilterByComponent(prs, cfg.Component) | ||
| } | ||
| FormatPipelineRuns(cfg.Stdout, prs) | ||
|
|
||
| printReleasePipeline(q, cfg.SHA, cfg.Component, cfg.Stdout, cfg.Stderr) | ||
|
|
||
| if HasPending(prs) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File outline\n'
ast-grep outline hack/tools/find-push-pipelinerun/watch.go --view expanded || true
printf '\n## watch.go (relevant section)\n'
sed -n '1,180p' hack/tools/find-push-pipelinerun/watch.go
printf '\n## tests mentioning WatchReleasePipeline / ListReleases\n'
rg -n "WatchReleasePipeline|ListReleases|WatchBuildPipeline|HasPendingReleases|HasPending" hack/tools/find-push-pipelinerun -n -A3 -B3
printf '\n## watch_test.go relevant section\n'
sed -n '1,260p' hack/tools/find-push-pipelinerun/watch_test.goRepository: openshift/hypershift
Length of output: 29818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,130p' hack/tools/find-push-pipelinerun/main.goRepository: openshift/hypershift
Length of output: 1345
Avoid fetching releases twice and swallowing API errors
WatchReleasePipeline already prints release data in the loop, so the extra ListReleases call only adds API traffic. More importantly, unexpected ListReleases failures are treated the same as “no release yet,” which can leave the watch spinning forever on RBAC/API errors. Return non-expected errors and only keep polling for the empty/not-created case.
🤖 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 `@hack/tools/find-push-pipelinerun/watch.go` around lines 40 - 58,
WatchReleasePipeline is fetching release data twice and hiding real API failures
by treating all ListReleases errors like “not found.” Update
WatchReleasePipeline and the printReleasePipeline path so releases are only
fetched once per loop, and distinguish expected empty/not-created cases from
unexpected errors. Use the existing Querier/ListReleases and
printReleasePipeline symbols to return non-expected errors immediately while
continuing to poll only for the missing-release case.
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 `@hack/tools/.golangci.yml`:
- Around line 1-46: The hack/tools/.golangci.yml config is currently unused
because the lint flow points at the root golangci config instead. Update the
lint setup so this file is actually invoked, either by adding a dedicated
hack/tools lint target or by wiring make lint to run golangci-lint with this
config; use the existing make lint entrypoint and the .golangci.yml file as the
locating symbols. If this config is not meant to be maintained separately,
remove it instead of leaving dead configuration behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c034d4c4-8d8e-4523-9443-f37f655ca623
📒 Files selected for processing (6)
hack/tools/.golangci.ymlhack/tools/find-push-pipelinerun/format.gohack/tools/find-push-pipelinerun/main.gohack/tools/find-push-pipelinerun/pr.gohack/tools/find-push-pipelinerun/query.gohack/tools/find-push-pipelinerun/watch.go
🚧 Files skipped from review as they are similar to previous changes (5)
- hack/tools/find-push-pipelinerun/query.go
- hack/tools/find-push-pipelinerun/pr.go
- hack/tools/find-push-pipelinerun/main.go
- hack/tools/find-push-pipelinerun/watch.go
- hack/tools/find-push-pipelinerun/format.go
| version: "2" | ||
| run: | ||
| allow-parallel-runners: true | ||
| linters: | ||
| enable: | ||
| - dupword | ||
| - durationcheck | ||
| - errorlint | ||
| - fatcontext | ||
| - gocyclo | ||
| - misspell | ||
| - nilerr | ||
| - noctx | ||
| - unparam | ||
| - usestdlibvars | ||
| settings: | ||
| gocyclo: | ||
| min-complexity: 30 | ||
| misspell: | ||
| locale: US | ||
| exclusions: | ||
| warn-unused: true | ||
| generated: lax | ||
| presets: | ||
| - comments | ||
| - common-false-positives | ||
| - legacy | ||
| - std-error-handling | ||
| rules: | ||
| - linters: | ||
| - unparam | ||
| path: '_test\.go$' | ||
| severity: | ||
| default: error | ||
| formatters: | ||
| enable: | ||
| - gci | ||
| settings: | ||
| gci: | ||
| sections: | ||
| - standard | ||
| - default | ||
| custom-order: true | ||
| exclusions: | ||
| warn-unused: true | ||
| generated: lax |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the Makefile defines a target that runs golangci-lint against hack/tools
rg -n "hack/tools" Makefile
rg -n "golangci-lint run" MakefileRepository: openshift/hypershift
Length of output: 1242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect lint-related Makefile targets and any CI entrypoints that call them.
rg -n "^(lint|.*lint.*):|golangci-lint|make lint|hack/tools" Makefile .github/workflows -g '!**/vendor/**' -g '!**/bin/**'
# Show the relevant Makefile section around lint targets.
line=$(rg -n "^(lint|.*lint.*):" Makefile | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-25))
[ "$start" -lt 1 ] && start=1
end=$((line+120))
sed -n "${start},${end}p" Makefile
fiRepository: openshift/hypershift
Length of output: 8795
Wire hack/tools/.golangci.yml into a lint target
make lint runs golangci-lint with the root ./.golangci.yml, so this config never gets applied. Add a hack/tools lint target or remove the file if it is unused.
🤖 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 `@hack/tools/.golangci.yml` around lines 1 - 46, The hack/tools/.golangci.yml
config is currently unused because the lint flow points at the root golangci
config instead. Update the lint setup so this file is actually invoked, either
by adding a dedicated hack/tools lint target or by wiring make lint to run
golangci-lint with this config; use the existing make lint entrypoint and the
.golangci.yml file as the locating symbols. If this config is not meant to be
maintained separately, remove it instead of leaving dead configuration behind.
- Add 30s timeout to GitHub and Kubernetes HTTP clients - Validate WATCH_INTERVAL is positive before using it - Consolidate ListReleases to one call per watch loop iteration - Surface ListReleases errors instead of silently continuing - Cache ReleasePlan mappings to avoid redundant API calls - Use http.MethodGet instead of string literal - Add golangci-lint config for hack/tools module Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9f27af0 to
91f7e26
Compare
|
The repo is public. The 403 was a transient access issue. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Konflux pipeline runs failed before any build or test task could execute. The Tekton pipeline resolver in the Konflux build system was unable to clone Root CauseThe root cause is a transient GitHub access failure from the Konflux build infrastructure. Specifically:
This failure is not caused by PR #8935's code changes. The pipeline never reached the stage where it would examine the PR's source code. Recommendations
Evidence
|
|
/override "ci/prow/e2e-azure-v2-self-managed" |
|
@celebdor: Overrode contexts on behalf of celebdor: ci/prow/e2e-azure-v2-self-managed, ci/prow/e2e-kubevirt-aws-ovn-reduced, ci/prow/e2e-v2-aws, ci/prow/e2e-v2-gke 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 kubernetes-sigs/prow repository. |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
@celebdor: 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. |
2 participants
Summary
find-push-pipelinerun.shbash script as a Go CLI tool athack/tools/find-push-pipelinerun/client-gofor Kubernetes auth and the GitHub REST API directly (nogh,jq,curldependencies)Querierdesign enables mock-driven unit tests for the watch loopReleasedcondition by type instead of assumingconditions[0]Test plan
cd hack/tools && go test ./find-push-pipelinerun/— 41/41 passgolangci-lint— 0 issueshack/tools/bin/find-push-pipelinerun 8923— push PipelineRuns displayed correctlyhack/tools/bin/find-push-pipelinerun --release 8923— releases and release PipelineRuns displayed correctly with destination imageshack/tools/bin/find-push-pipelinerun --release --watch <recent-PR>— poll modeJira
CNTRLPLANE-3773
🤖 Generated with Claude Code
Summary by CodeRabbit
find-push-pipelinerunto locate merged PR-triggered PipelineRuns by merge commit SHA, with optional component filtering and--watchpolling.--releasemode to display release pipeline status and expected destination images, with live-to-archive fallback.