Release Qualifying Jobs Support#775
Open
bradmwilliams wants to merge 4 commits into
Open
Conversation
Collaborator
Author
|
/hold |
Contributor
📝 WalkthroughWalkthroughAdds a release-qualifiers subsystem: YAML config loading with live-reload, validation/merge/pretty-print utilities, BigQuery client + cached wrapper and prowjob query builder, qualifier summary generation, two controllers (qualifiers summary and Jira escalations), API/UI wiring for qualifier badges/status, and extensive tests. ChangesRelease Qualifiers Feature
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bradmwilliams 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
pkg/releasequalifiers/config_loader.go (1)
55-65: 💤 Low valueClarify copy semantics for shared pointer fields.
Get()performs a shallow copy of theReleaseQualifiersmap, copying eachReleaseQualifierstruct by value. However, pointer fields within the struct (Enabled,Approval,Notifications) remain shared between the returned copy and the internal config. While this is likely intentional for efficiency, external modification of nested notification escalations or other pointer-backed fields could affect internal state.If strict immutability is required, consider deep-copying pointer fields or documenting the shallow-copy behavior in a comment above
Get().🤖 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 `@pkg/releasequalifiers/config_loader.go` around lines 55 - 65, Get() currently returns a shallow copy of the ReleaseQualifiers map (function ConfigLoader.Get and type ReleaseQualifiers), which copies each ReleaseQualifier value but leaves pointer fields (Enabled, Approval, Notifications) shared with the internal state; either implement a deep copy of those pointer-backed fields inside Get() (clone each ReleaseQualifier and allocate new copies for Enabled, Approval, Notifications and any nested structs/slices/maps) to ensure immutability, or add a clear comment above ConfigLoader.Get() documenting the shallow-copy semantics and that callers must not mutate pointer-backed fields; update the code accordingly for whichever approach you choose.cmd/release-controller/sync_release_payload_test.go (1)
27-40: ⚡ Quick winAdd explicit qualifier propagation assertions in
TestNewReleasePayload.
newReleasePayloadnow copiesverificationJobDefinition.Qualifiersinto generated CI jobs, but this table does not assert that behavior yet. A small case with non-empty qualifiers (including aggregated jobs) would prevent silent regressions.Suggested test addition
+{ + name: "BlockingAndAggregatedJobsPropagateQualifiers", + release: release, + payloadName: "4.11.0-0.nightly-2022-03-11-113341", + jobNamespace: "ci-release", + prowNamespace: "ci", + verificationJobs: map[string]releasecontroller.ReleaseVerification{ + "aggregated-job": { + ProwJob: &releasecontroller.ProwJobVerification{Name: "periodic-ci-foo"}, + AggregatedProwJob: &releasecontroller.AggregatedProwJobVerification{AnalysisJobCount: 2}, + Qualifiers: releasequalifiers.ReleaseQualifiers{ + "some-qualifier": {}, + }, + }, + }, + upgradeJobs: map[string]releasecontroller.UpgradeVerification{}, + dataSource: v1alpha1.PayloadVerificationDataSourceBuildFarm, + payloadType: v1alpha1.PayloadTypeLocal, + expected: &v1alpha1.ReleasePayload{ + // ... + Spec: v1alpha1.ReleasePayloadSpec{ + // ... + PayloadVerificationConfig: v1alpha1.PayloadVerificationConfig{ + BlockingJobs: []v1alpha1.CIConfiguration{{ + CIConfigurationName: "aggregated-job", + CIConfigurationJobName: "aggregated-job-release-openshift-release-analysis-aggregator", + Qualifiers: releasequalifiers.ReleaseQualifiers{"some-qualifier": {}}, + }}, + InformingJobs: []v1alpha1.CIConfiguration{{ + CIConfigurationName: "aggregated-job", + CIConfigurationJobName: "periodic-ci-foo", + AnalysisJobCount: 2, + Qualifiers: releasequalifiers.ReleaseQualifiers{"some-qualifier": {}}, + }}, + }, + }, + }, +},Also applies to: 567-579, 777-825
🤖 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/release-controller/sync_release_payload_test.go` around lines 27 - 40, TestNewReleasePayload currently lacks assertions that newReleasePayload copies verificationJobDefinition.Qualifiers into generated CI jobs; add a table test case entry in TestNewReleasePayload that supplies a releaseVerification/upgradeVerification whose verificationJobDefinition.Qualifiers is non-empty (including at least one aggregated job case), call newReleasePayload via the existing test scaffolding, and assert that the produced v1alpha1.ReleasePayload contains the same qualifier values on the generated CI job entries (both individual and aggregated job representations) so qualifiers are preserved; reference verificationJobDefinition.Qualifiers, newReleasePayload, and the test table row for where to add the case and assertions.pkg/jira/jira.go (1)
249-249: ⚡ Quick winConsider extracting bot identity check to a helper.
The multi-part author check is complex and spans the entire line. Extracting to a helper function would improve readability and make it easier to add new bot identities in the future.
♻️ Suggested refactor
+func isReleaseControllerBot(author *jiraBaseClient.User) bool { + return author.Name == "openshift-crt-jira-release-controller" || + author.EmailAddress == "brawilli+openshift-crt-jira-release-controller@redhat.com" || + author.EmailAddress == "brawilli@redhat.com" +} + func (c *Verifier) commentIssue(errs *[]error, issue *jiraBaseClient.Issue, message string) { if message == "" { return } for _, comment := range issue.Fields.Comments.Comments { - if (comment.Body == message || strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) && (comment.Author.Name == "openshift-crt-jira-release-controller" || comment.Author.EmailAddress == "brawilli+openshift-crt-jira-release-controller@redhat.com" || comment.Author.EmailAddress == "brawilli@redhat.com") { + if (comment.Body == message || strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) && isReleaseControllerBot(comment.Author) { 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 `@pkg/jira/jira.go` at line 249, The long inline bot identity check should be extracted into a helper (e.g., add a function isBotAuthor(author *jira.User) bool) and then replace the big expression in the if with a clearer call (e.g., if (comment.Body == message || strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) && isBotAuthor(comment.Author) { ). Implement isBotAuthor to test Name and EmailAddress against the current known bot identities (openshift-crt-jira-release-controller and both brawilli emails), document or centralize the list so new bot identities can be added easily, and update any affected callers/tests to use the new helper.
🤖 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/release-controller-api/http_helper.go`:
- Around line 529-538: getJobQualifierMap currently appends qualifier IDs to
result[job.CIConfigurationName] without deduplication, causing duplicate badges;
fix by collecting qualifier IDs into a per-job set (e.g., map[string]struct{})
instead of a simple slice, then convert that set to a slice, sort it, and assign
back to result so each job key has unique, deterministically-ordered qualifier
IDs. Locate the loop over payload.Status.QualifiersSummary.Qualifiers and the
inner loop using job.CIConfigurationName and replace the append logic with set
insertion, then perform the slice conversion + sort (the same change should be
applied to the similar block referenced around the other occurrence).
In `@cmd/release-controller-api/http.go`:
- Around line 1953-1962: The job identity currently keys jobStatusMap by
CIConfigurationName causing collisions; update all places that build or lookup
jobStatusMap (e.g., where jobStatusMap := make(map[string]*v1alpha1.JobStatus)
and the loops over payload.Status.BlockingJobResults and
payload.Status.InformingJobResults) to use a composite key combining
CIConfigurationName and CIConfigurationJobName (for example join with a
separator) and update every lookup site (the other map constructions and
accesses referenced around the payload handling and rows rendering) to compute
and use the same composite key so distinct jobs (including aggregated ones) are
kept separate.
In `@cmd/release-controller-api/static/releaseStreamPageHtml.tmpl`:
- Line 35: The template is escaping HTML because qualifierBadges returns a plain
string; update the generator function qualifierBadges to return template.HTML
(not string) so html/template treats its output as trusted HTML, and adjust any
callers/tests to accept the new return type; ensure the function still sanitizes
or constructs safe markup before converting to template.HTML to avoid XSS.
In `@pkg/bigquery/cached_client.go`:
- Around line 44-74: GetReleaseQualifiersProwjobSummaryWithFilters in
CachedClient currently only checks expiry on reads but never removes expired
cachedEntry items, causing unbounded growth; fix by evicting expired entries
either lazily during writes or via a background janitor: inside
CachedClient.GetReleaseQualifiersProwjobSummaryWithFilters, before inserting the
new entry (or when you detect a cache miss), take c.mu.Lock and iterate c.cache
to delete entries whose expiresAt.Before(time.Now()), then insert the new
cachedEntry (use c.ttl as before) and unlock; alternatively add a startCleanup
goroutine on CachedClient construction that periodically locks c.mu and purges
expired cachedEntry items to keep the map bounded.
In `@pkg/bigquery/prowjobs_test.go`:
- Around line 254-266: Tests use inconsistent job-name quoting which masks a
SQL-injection vulnerability in BuildProwjobSummaryQuery; change the API and
tests to treat jobNames as raw unquoted identifiers (e.g.,
[]string{"job-a","job-b"}) and update BuildProwjobSummaryQuery to produce a
parameterized query (use placeholders or a safe query builder) rather than
injecting pre-quoted strings, then update tests (and FakeClient matcher) to
assert on the parameterized form: check for the IN clause placeholder pattern
(or number of placeholders) and that prowjob_completion and the default INTERVAL
14 DAY appear, and update any other tests (lines 21–47 and the cited case) to
pass unquoted names and validate SQL correctness instead of exact string
equality.
In `@pkg/bigquery/prowjobs.go`:
- Around line 61-94: BuildProwjobSummaryQuery currently injects unescaped job
names (defaultJobs and filteredJobs[].Name) into the SQL string, enabling SQL
injection; change it to use BigQuery query parameters instead of string
interpolation: accept a []bigquery.QueryParameter (or
bigquery.QueryConfig.Parameters) and replace the IN list with "prowjob_job_name
IN UNNEST(`@default_jobs`)" and the per-filter comparisons with parameterized
expressions (e.g., "prowjob_job_name = `@filtered_name_i` AND prowjob_start >=
DATETIME_SUB(CURRENT_DATETIME(), INTERVAL `@interval_i`)"), populate `@default_jobs`
with the slice of job names and create distinct parameters for each filtered job
name and interval (or combine filtered names/intervals into arrays), and
continue to validate/normalize intervals from parsePeriodToSQLInterval if you
keep embedding them as strings; update the caller to execute the query via
bigquery.Client.Query with those parameters instead of passing a raw formatted
SQL string.
In `@pkg/cmd/release-payload-controller/job_state_controller.go`:
- Around line 90-100: The loop currently assigns to a copy (jobResults) so
calling jobstatus.SetJobStatus(&jobResults, job) mutates the address of the loop
variable rather than the original slice; change the iteration to operate on the
actual slice fields (e.g., iterate by index or obtain pointers to
releasePayload.Status.BlockingJobResults / InformingJobResults /
UpgradeJobResults) and pass the address of the real slice variable into
jobstatus.SetJobStatus. Ensure you still compute Job.AggregateState via
computeJobState(job) and then call jobstatus.SetJobStatus on the original slice
(not the loop-copy) so updates to Job.AggregateState persist.
In `@pkg/releasequalifiers/merge.go`:
- Around line 13-21: The Merge method on ReleaseQualifiers currently builds only
the intersection; change it to produce the union by initializing result, first
copying all entries from rqs into result, then iterating overrides and for each
qualifierId either set result[qualifierId] = override (or result[qualifierId] =
mergeQualifier(result[qualifierId], override) if key exists) so overrides take
precedence or are merged; update ReleaseQualifiers.Merge to use mergeQualifier
when both sides exist and to include keys present only in one map.
---
Nitpick comments:
In `@cmd/release-controller/sync_release_payload_test.go`:
- Around line 27-40: TestNewReleasePayload currently lacks assertions that
newReleasePayload copies verificationJobDefinition.Qualifiers into generated CI
jobs; add a table test case entry in TestNewReleasePayload that supplies a
releaseVerification/upgradeVerification whose
verificationJobDefinition.Qualifiers is non-empty (including at least one
aggregated job case), call newReleasePayload via the existing test scaffolding,
and assert that the produced v1alpha1.ReleasePayload contains the same qualifier
values on the generated CI job entries (both individual and aggregated job
representations) so qualifiers are preserved; reference
verificationJobDefinition.Qualifiers, newReleasePayload, and the test table row
for where to add the case and assertions.
In `@pkg/jira/jira.go`:
- Line 249: The long inline bot identity check should be extracted into a helper
(e.g., add a function isBotAuthor(author *jira.User) bool) and then replace the
big expression in the if with a clearer call (e.g., if (comment.Body == message
|| strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) &&
isBotAuthor(comment.Author) { ). Implement isBotAuthor to test Name and
EmailAddress against the current known bot identities
(openshift-crt-jira-release-controller and both brawilli emails), document or
centralize the list so new bot identities can be added easily, and update any
affected callers/tests to use the new helper.
In `@pkg/releasequalifiers/config_loader.go`:
- Around line 55-65: Get() currently returns a shallow copy of the
ReleaseQualifiers map (function ConfigLoader.Get and type ReleaseQualifiers),
which copies each ReleaseQualifier value but leaves pointer fields (Enabled,
Approval, Notifications) shared with the internal state; either implement a deep
copy of those pointer-backed fields inside Get() (clone each ReleaseQualifier
and allocate new copies for Enabled, Approval, Notifications and any nested
structs/slices/maps) to ensure immutability, or add a clear comment above
ConfigLoader.Get() documenting the shallow-copy semantics and that callers must
not mutate pointer-backed fields; update the code accordingly for whichever
approach you choose.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 93d30026-8835-474f-941a-24628dd5cd55
⛔ Files ignored due to path filters (257)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/bigquery/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/arrow.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/bigquery.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/bigquery.replayis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/copy.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/dataset.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/error.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/external.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/extract.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/file.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/gcs.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/iam.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/inserter.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/internal/query/order.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/internal/version.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/intervalvalue.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/iterator.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/job.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/load.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/model.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/nulls.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/params.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/query.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/random.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/rangevalue.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/routine.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/schema.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/standardsql.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/auxiliary.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/auxiliary_go123.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/big_query_read_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/big_query_write_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/helpers.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/info.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/annotations.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/arrow.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/avro.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/protobuf.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/storage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/stream.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/table.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/version.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage_iterator.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/table.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/value.gois excluded by!vendor/**vendor/cloud.google.com/go/civil/civil.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/detect/detect.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/fields/fields.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/fields/fold.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/uid/uid.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/LICENSE.txtis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/.editorconfigis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/.gitignoreis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Gopkg.lockis excluded by!**/*.lock,!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Gopkg.tomlis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/array.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/binarybuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/boolean.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/booleanbuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_byte.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/compare.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/concat.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/data.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/decimal128.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/decimal256.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/dictionary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/diff.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/encoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/extension.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/extension_builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixed_size_list.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixedsize_binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixedsize_binarybuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/float16.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/float16_builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/interval.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/json_reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/list.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/map.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen_test.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/record.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/string.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/struct.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/table.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/timestamp.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/union.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/arrio/arrio.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_avx2_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_avx2_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_noasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_ppc64le.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_s390x.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_sse4_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_sse4_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmaps.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitutil.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/endian_default.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/endian_s390x.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/compare.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_encoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_extension.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_fixedwidth.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_nested.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.go.tmpldatais excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline_go1.19.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline_tinygo.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/decimal128/decimal128.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/decimal256/decimal256.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/encoded/ree_utils.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/big.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/endian.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/little.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/errors.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/float16/float16.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/assert_off.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/assert_on.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/log_off.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/log_on.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/dictutils/dict.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BinaryView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Block.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BodyCompression.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BodyCompressionMethod.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Bool.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Buffer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/CompressionType.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Date.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DateUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Decimal.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryBatch.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryEncoding.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryKind.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Duration.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Endianness.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Feature.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Field.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FieldNode.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FixedSizeBinary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FixedSizeList.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FloatingPoint.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Footer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Int.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Interval.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/IntervalUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/KeyValue.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeBinary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeList.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeListView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeUtf8.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/List.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/ListView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Map.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Message.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/MessageHeader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/MetadataVersion.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Precision.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RecordBatch.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RunEndEncoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RunLengthEncoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Schema.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixCompressedAxis.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixIndexCSR.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixIndexCSX.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensor.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndex.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndexCOO.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndexCSF.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Struct_.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Tensor.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/TensorDim.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Time.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/TimeUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Timestamp.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Type.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Union.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/UnionMode.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Utf8.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Utf8View.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/utils.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/compression.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/endian_swap.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/file_reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/file_writer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/ipc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/message.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/metadata.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/writer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/buffer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator_defaults.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator_logging.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/checked_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/default_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/default_mallocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/go_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.ccis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.his excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/helpers.his excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/mallocator/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/mallocator/mallocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_avx2_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_avx2_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_js_wasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_neon_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_neon_arm64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_noasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_sse4_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_sse4_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/numeric.schema.jsonis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/numeric.tmpldatais excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/record.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/schema.gois excluded by!vendor/**
📒 Files selected for processing (43)
cmd/release-controller-api/controller.gocmd/release-controller-api/http.gocmd/release-controller-api/http_helper.gocmd/release-controller-api/main.gocmd/release-controller-api/static/css/custom.csscmd/release-controller-api/static/releasePageHtml.tmplcmd/release-controller-api/static/releaseStreamPageHtml.tmplcmd/release-controller/config_validate.gocmd/release-controller/config_validate_test.gocmd/release-controller/sync_release_payload.gocmd/release-controller/sync_release_payload_test.gogo.modpkg/bigquery/bigquery.gopkg/bigquery/bigquery_test.gopkg/bigquery/cached_client.gopkg/bigquery/cached_client_test.gopkg/bigquery/fake.gopkg/bigquery/prowjobs.gopkg/bigquery/prowjobs_test.gopkg/cmd/release-payload-controller/cmd.gopkg/cmd/release-payload-controller/jira_escalations_controller.gopkg/cmd/release-payload-controller/jira_escalations_controller_test.gopkg/cmd/release-payload-controller/job_state_controller.gopkg/cmd/release-payload-controller/payload_verification_controller_test.gopkg/cmd/release-payload-controller/qualifiers_summary_controller.gopkg/cmd/release-payload-controller/qualifiers_summary_controller_test.gopkg/cmd/release-payload-controller/release_creation_status_controller_test.gopkg/jira/jira.gopkg/release-controller/types.gopkg/releasepayload/qualifiers/helpers.gopkg/releasepayload/qualifiers/helpers_test.gopkg/releasepayload/v1alpha1helpers/helpers.gopkg/releasepayload/v1alpha1helpers/helpers_test.gopkg/releasequalifiers/config_loader.gopkg/releasequalifiers/config_loader_test.gopkg/releasequalifiers/merge.gopkg/releasequalifiers/merge_test.gopkg/releasequalifiers/notifications/jira/escalation_test.gopkg/releasequalifiers/prettyprint.gopkg/releasequalifiers/prettyprint_test.gopkg/releasequalifiers/types.gopkg/releasequalifiers/validation.gopkg/releasequalifiers/validation_test.go
💤 Files with no reviewable changes (1)
- pkg/release-controller/types.go
Comment on lines
+254
to
+266
| t.Run("default jobs only - matches legacy format", func(t *testing.T) { | ||
| query := BuildProwjobSummaryQuery("proj", []string{"'job-a'", "'job-b'"}, nil) | ||
|
|
||
| if !strings.Contains(query, "prowjob_job_name IN ('job-a','job-b')") { | ||
| t.Errorf("expected IN clause with both jobs, got:\n%s", query) | ||
| } | ||
| if !strings.Contains(query, "INTERVAL 14 DAY") { | ||
| t.Error("expected 14 DAY default interval") | ||
| } | ||
| if !strings.Contains(query, "prowjob_completion") { | ||
| t.Error("expected prowjob_completion in SELECT") | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Inconsistent test data reveals unclear API contract.
The tests show inconsistent expectations for BuildProwjobSummaryQuery:
- Line 24:
jobNames: []string{"periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-upgrade-fips-no-nat-instance"}(unquoted) - Line 255:
BuildProwjobSummaryQuery("proj", []string{"'job-a'", "'job-b'"}, nil)(pre-quoted)
This inconsistency stems from the underlying SQL injection issue in BuildProwjobSummaryQuery. The tests pass because FakeClient matches exact query strings without validating SQL syntax.
Once the SQL injection vulnerability is fixed with parameterized queries, this API contract will need clarification and the tests should validate SQL correctness, not just string matching.
Also applies to: 21-47
🤖 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 `@pkg/bigquery/prowjobs_test.go` around lines 254 - 266, Tests use inconsistent
job-name quoting which masks a SQL-injection vulnerability in
BuildProwjobSummaryQuery; change the API and tests to treat jobNames as raw
unquoted identifiers (e.g., []string{"job-a","job-b"}) and update
BuildProwjobSummaryQuery to produce a parameterized query (use placeholders or a
safe query builder) rather than injecting pre-quoted strings, then update tests
(and FakeClient matcher) to assert on the parameterized form: check for the IN
clause placeholder pattern (or number of placeholders) and that
prowjob_completion and the default INTERVAL 14 DAY appear, and update any other
tests (lines 21–47 and the cited case) to pass unquoted names and validate SQL
correctness instead of exact string equality.
Comment on lines
+90
to
100
| // Update the Job.AggregateState | ||
| for _, jobResults := range [][]v1alpha1.JobStatus{ | ||
| releasePayload.Status.BlockingJobResults, | ||
| releasePayload.Status.InformingJobResults, | ||
| releasePayload.Status.UpgradeJobResults, | ||
| } { | ||
| for _, job := range jobResults { | ||
| job.AggregateState = computeJobState(job) | ||
| jobstatus.SetJobStatus(&jobResults, job) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Loop variable address prevents job state updates from persisting.
The refactored loop passes &jobResults to SetJobStatus, but jobResults is a loop variable containing a copy of each slice, not the original slice from releasePayload.Status. Taking the address of the loop variable means updates won't persist to the payload.
When the loop iterates, jobResults is reassigned to the next slice value, and any modifications via the stale &jobResults pointer are lost.
🔧 Recommended fix: use slice pointers
- // Update the Job.AggregateState
- for _, jobResults := range [][]v1alpha1.JobStatus{
- releasePayload.Status.BlockingJobResults,
- releasePayload.Status.InformingJobResults,
- releasePayload.Status.UpgradeJobResults,
- } {
- for _, job := range jobResults {
+ for _, jobResultsPtr := range []*[]v1alpha1.JobStatus{
+ &releasePayload.Status.BlockingJobResults,
+ &releasePayload.Status.InformingJobResults,
+ &releasePayload.Status.UpgradeJobResults,
+ } {
+ for _, job := range *jobResultsPtr {
job.AggregateState = computeJobState(job)
- jobstatus.SetJobStatus(&jobResults, job)
+ jobstatus.SetJobStatus(jobResultsPtr, job)
}
}🤖 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 `@pkg/cmd/release-payload-controller/job_state_controller.go` around lines 90 -
100, The loop currently assigns to a copy (jobResults) so calling
jobstatus.SetJobStatus(&jobResults, job) mutates the address of the loop
variable rather than the original slice; change the iteration to operate on the
actual slice fields (e.g., iterate by index or obtain pointers to
releasePayload.Status.BlockingJobResults / InformingJobResults /
UpgradeJobResults) and pass the address of the real slice variable into
jobstatus.SetJobStatus. Ensure you still compute Job.AggregateState via
computeJobState(job) and then call jobstatus.SetJobStatus on the original slice
(not the loop-copy) so updates to Job.AggregateState persist.
Comment on lines
+13
to
+21
| func (rqs ReleaseQualifiers) Merge(overrides ReleaseQualifiers) ReleaseQualifiers { | ||
| result := make(ReleaseQualifiers) | ||
| for qualifierId, qualifier := range rqs { | ||
| if override, exists := overrides[qualifierId]; exists { | ||
| result[qualifierId] = mergeQualifier(qualifier, override) | ||
| } | ||
| } | ||
| return result | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Merge returns intersection instead of union.
The current implementation only includes qualifiers that exist in BOTH rqs and overrides, silently dropping qualifiers that appear in only one map. This contradicts the documented behavior ("returns a new ReleaseQualifiers containing the union of both").
For example, if rqs contains {"a": {...}} and overrides contains {"b": {...}}, the result will be empty instead of {"a": {...}, "b": {...}}.
🐛 Proposed fix to return the union
func (rqs ReleaseQualifiers) Merge(overrides ReleaseQualifiers) ReleaseQualifiers {
result := make(ReleaseQualifiers)
+
+ // First, copy all base qualifiers
for qualifierId, qualifier := range rqs {
- if override, exists := overrides[qualifierId]; exists {
- result[qualifierId] = mergeQualifier(qualifier, override)
- }
+ result[qualifierId] = qualifier
+ }
+
+ // Then, merge or add override qualifiers
+ for qualifierId, override := range overrides {
+ if base, exists := result[qualifierId]; exists {
+ result[qualifierId] = mergeQualifier(base, override)
+ } else {
+ result[qualifierId] = override
+ }
}
+
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (rqs ReleaseQualifiers) Merge(overrides ReleaseQualifiers) ReleaseQualifiers { | |
| result := make(ReleaseQualifiers) | |
| for qualifierId, qualifier := range rqs { | |
| if override, exists := overrides[qualifierId]; exists { | |
| result[qualifierId] = mergeQualifier(qualifier, override) | |
| } | |
| } | |
| return result | |
| } | |
| func (rqs ReleaseQualifiers) Merge(overrides ReleaseQualifiers) ReleaseQualifiers { | |
| result := make(ReleaseQualifiers) | |
| // First, copy all base qualifiers | |
| for qualifierId, qualifier := range rqs { | |
| result[qualifierId] = qualifier | |
| } | |
| // Then, merge or add override qualifiers | |
| for qualifierId, override := range overrides { | |
| if base, exists := result[qualifierId]; exists { | |
| result[qualifierId] = mergeQualifier(base, override) | |
| } else { | |
| result[qualifierId] = override | |
| } | |
| } | |
| return result | |
| } |
🤖 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 `@pkg/releasequalifiers/merge.go` around lines 13 - 21, The Merge method on
ReleaseQualifiers currently builds only the intersection; change it to produce
the union by initializing result, first copying all entries from rqs into
result, then iterating overrides and for each qualifierId either set
result[qualifierId] = override (or result[qualifierId] =
mergeQualifier(result[qualifierId], override) if key exists) so overrides take
precedence or are merged; update ReleaseQualifiers.Merge to use mergeQualifier
when both sides exist and to include keys present only in one map.
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
bradmwilliams
force-pushed
the
release-qualifying-jobs-support
branch
from
fdee0e2 to
73b35ea
Compare
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
Collaborator
Author
|
/label tide/merge-method/squash |
Contributor
|
@bradmwilliams: The label(s) 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. |
Collaborator
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pkg/releasequalifiers/merge.go (1)
10-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix misleading documentation comment.
The comment states "returns a new ReleaseQualifiers containing the union of both," but the implementation returns only qualifiers present in
rqs(the receiver), with overrides applied from theoverridesparameter for matching keys. Qualifiers that exist only inoverridesare not included in the result.This is confirmed by tests (e.g.,
merge_test.go:30-39expects empty result when base is empty) and downstream usage patterns where global config defines available qualifiers and job-level config provides overrides.📝 Proposed documentation fix
-// Merge takes a ReleaseQualifiers object and returns a new ReleaseQualifiers containing the union of both -// Override values take precedence when both are defined +// Merge returns a new ReleaseQualifiers containing qualifiers from the receiver (rqs) +// with field-level overrides applied from the overrides parameter for matching qualifier IDs. +// Qualifiers present only in overrides (not in rqs) are not included. // Deep merge is performed for nested structures🤖 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 `@pkg/releasequalifiers/merge.go` around lines 10 - 21, The comment for ReleaseQualifiers.Merge is misleading: it claims to return the union of both maps but the implementation of Merge (receiver rqs) only preserves keys from rqs and applies matching entries from overrides via mergeQualifier; entries present only in overrides are not added. Update the doc comment on the Merge method to accurately describe its behavior (e.g., "returns a new ReleaseQualifiers containing entries from the receiver with matching entries from overrides applied via mergeQualifier; keys present only in overrides are not included"), and mention that override values take precedence and deep merge is performed by mergeQualifier.
🤖 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/release-controller-api/http.go`:
- Around line 1849-1858: The qualifierStateText function returns an empty string
for approval qualifiers in the default branch, causing pending approvals to
render blank; update qualifierStateText (which inspects summary.Approval and
summary.AggregateState) so that the default case for approval returns a stable
label such as "Pending" or "Awaiting approval" instead of "" (preserve existing
returns for v1alpha1.JobStateSuccess and v1alpha1.JobStateFailure).
- Around line 2638-2673: After locating jobConfig, explicitly verify that the
job actually references the qualifier by checking jobConfig.Qualifiers for
qualifierID and return a 404 if it is not present; do this before using the
global config. In other words, after finding jobConfig (symbol: jobConfig) add a
presence check for qualifierID in jobConfig.Qualifiers (use the map lookup
pattern), and if not found call http.Error(...) with StatusNotFound and return;
only then proceed to build mergedQualifier by loading the global config via
c.configAccessor.Get(), applying the global base (gq) and then Merge the job
override (mergedQualifier.Merge) from jobConfig.Qualifiers[qualifierID].
- Around line 1953-1962: The job status lookup and qualifier config search
currently omit upgrade jobs; update the jobStatusMap population in
cmd/release-controller-api/http.go to also iterate
payload.Status.UpgradeJobResults (in addition to BlockingJobResults and
InformingJobResults) so upgrade job statuses/attempts are available, and update
the qualifier config handler (the logic that searches
payload.Spec.PayloadVerificationConfig.BlockingJobs and InformingJobs) to also
search payload.Spec.PayloadVerificationConfig.UpgradeJobs (and ensure
GenerateQualifiersSummary uses
payload.Spec.PayloadVerificationConfig.UpgradeJobs when building summary.Jobs)
so upgrade jobs are present in the detail and config endpoints rather than
returning “Job ... not found” or rendering “-”.
In `@pkg/cmd/release-payload-controller/jira_escalations_controller.go`:
- Around line 176-215: In collectFromJobs, job.Qualifiers is iterated
nondeterministically and you mark seen and break on the first Jira-enabled
qualifier found; instead compute a deterministic superset interval for the job
by scanning all qualifierIDs for that job (using qualifiersConfig and
job.Qualifiers), merging each qualifier (globalConfig.Merge(jobQualifierConfig))
and collecting all non-empty merged.Notifications.Jira.Escalations OverPeriod
values, converting each with parsePeriodToSQLInterval and taking the largest
interval (superset) before setting seen[job.CIConfigurationJobName]; then push
either a single bigquery.ProwjobQueryFilter with that superset interval onto
filteredJobs or fall back to defaultJobs if none valid, and ensure
signing/logging of parse errors per-qualifier while continuing the superset
computation.
- Around line 289-308: The code marks an escalation active even when
triggerJiraEscalation(...) failed (returns empty issueKey), causing retries to
be suppressed; change the logic in the block that handles highestEscalation so
that you only set notifState.ActiveEscalation, notifState.ActivePriority,
notifState.Abated=false, notifState.LastTransitionTime and call
c.setNotificationState(...) when triggerJiraEscalation(...) returns a non-empty
issueKey (or propagate an error from triggerJiraEscalation to requeue the
reconcile); specifically, check issueKey before mutating notifState and
persisting via setNotificationState, keeping the duplicate-suppression behavior
only for successfully-created/updated Jira issues.
In `@pkg/jira/jira.go`:
- Around line 246-250: The deduplication predicate inside the loop over
issue.Fields.Comments.Comments is missing the legacy bot email and therefore can
create duplicate verification comments; update the condition that checks
comment.Author.EmailAddress (alongside comment.Author.Name and comment.Body and
issue.Fields.Status.Name/jira.StatusVerified) to also accept
"openshift-release-controller-jira-bot@redhat.com" in the OR list so older
comments by that legacy identity are treated as bot comments and cause an early
return.
---
Duplicate comments:
In `@pkg/releasequalifiers/merge.go`:
- Around line 10-21: The comment for ReleaseQualifiers.Merge is misleading: it
claims to return the union of both maps but the implementation of Merge
(receiver rqs) only preserves keys from rqs and applies matching entries from
overrides via mergeQualifier; entries present only in overrides are not added.
Update the doc comment on the Merge method to accurately describe its behavior
(e.g., "returns a new ReleaseQualifiers containing entries from the receiver
with matching entries from overrides applied via mergeQualifier; keys present
only in overrides are not included"), and mention that override values take
precedence and deep merge is performed by mergeQualifier.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab80d905-b4a3-4b4a-927e-e43afdc4c5e8
⛔ Files ignored due to path filters (256)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/bigquery/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/arrow.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/bigquery.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/bigquery.replayis excluded by!vendor/**vendor/cloud.google.com/go/bigquery/copy.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/dataset.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/error.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/external.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/extract.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/file.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/gcs.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/iam.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/inserter.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/internal/query/order.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/internal/version.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/intervalvalue.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/iterator.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/job.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/load.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/model.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/nulls.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/params.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/query.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/random.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/rangevalue.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/routine.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/schema.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/standardsql.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/auxiliary.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/auxiliary_go123.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/big_query_read_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/big_query_write_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/helpers.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/info.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/annotations.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/arrow.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/avro.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/protobuf.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/storage.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/stream.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/storagepb/table.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/bigquery/storage/apiv1/version.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage_client.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/storage_iterator.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/table.gois excluded by!vendor/**vendor/cloud.google.com/go/bigquery/value.gois excluded by!vendor/**vendor/cloud.google.com/go/civil/civil.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/detect/detect.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/fields/fields.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/fields/fold.gois excluded by!vendor/**vendor/cloud.google.com/go/internal/uid/uid.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/LICENSE.txtis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/.editorconfigis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/.gitignoreis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Gopkg.lockis excluded by!**/*.lock,!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Gopkg.tomlis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/array.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/binarybuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/boolean.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/booleanbuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_byte.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/bufferbuilder_numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/compare.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/concat.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/data.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/decimal128.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/decimal256.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/dictionary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/diff.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/encoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/extension.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/extension_builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixed_size_list.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixedsize_binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/fixedsize_binarybuilder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/float16.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/float16_builder.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/interval.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/json_reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/list.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/map.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/numericbuilder.gen_test.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/record.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/string.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/struct.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/table.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/timestamp.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/union.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/array/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/arrio/arrio.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_avx2_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_avx2_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_noasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_ppc64le.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_s390x.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_sse4_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmap_ops_sse4_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitmaps.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/bitutil.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/endian_default.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/bitutil/endian_s390x.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/compare.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_encoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_extension.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_fixedwidth.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_nested.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.go.tmplis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_numeric.gen.go.tmpldatais excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline_go1.19.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/datatype_viewheader_inline_tinygo.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/decimal128/decimal128.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/decimal256/decimal256.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/encoded/ree_utils.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/big.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/endian.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/endian/little.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/errors.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/float16/float16.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/assert_off.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/assert_on.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/log_off.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/log_on.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/debug/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/dictutils/dict.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Binary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BinaryView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Block.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BodyCompression.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/BodyCompressionMethod.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Bool.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Buffer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/CompressionType.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Date.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DateUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Decimal.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryBatch.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryEncoding.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/DictionaryKind.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Duration.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Endianness.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Feature.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Field.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FieldNode.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FixedSizeBinary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FixedSizeList.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/FloatingPoint.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Footer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Int.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Interval.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/IntervalUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/KeyValue.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeBinary.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeList.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeListView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/LargeUtf8.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/List.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/ListView.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Map.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Message.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/MessageHeader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/MetadataVersion.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Null.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Precision.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RecordBatch.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RunEndEncoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/RunLengthEncoded.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Schema.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixCompressedAxis.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixIndexCSR.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseMatrixIndexCSX.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensor.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndex.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndexCOO.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/SparseTensorIndexCSF.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Struct_.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Tensor.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/TensorDim.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Time.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/TimeUnit.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Timestamp.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Type.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Union.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/UnionMode.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Utf8.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/flatbuf/Utf8View.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/internal/utils.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/compression.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/endian_swap.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/file_reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/file_writer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/ipc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/message.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/metadata.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/reader.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/ipc/writer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/Makefileis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/buffer.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator_defaults.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/cgo_allocator_logging.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/checked_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/default_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/default_mallocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/go_allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.ccis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/allocator.his excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/internal/cgoalloc/helpers.his excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/mallocator/doc.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/mallocator/mallocator.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_avx2_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_avx2_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_js_wasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_neon_arm64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_neon_arm64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_noasm.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_sse4_amd64.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/memory_sse4_amd64.sis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/memory/util.gois excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/numeric.schema.jsonis excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/numeric.tmpldatais excluded by!vendor/**vendor/github.com/apache/arrow/go/v15/arrow/record.gois excluded by!vendor/**
📒 Files selected for processing (44)
cmd/release-controller-api/controller.gocmd/release-controller-api/http.gocmd/release-controller-api/http_helper.gocmd/release-controller-api/main.gocmd/release-controller-api/static/css/custom.csscmd/release-controller-api/static/releasePageHtml.tmplcmd/release-controller-api/static/releaseStreamPageHtml.tmplcmd/release-controller/config_validate.gocmd/release-controller/config_validate_test.gocmd/release-controller/sync_release_payload.gocmd/release-controller/sync_release_payload_test.godata/results/bquxjob_65df743f_19de556b6a3.jsongo.modpkg/bigquery/bigquery.gopkg/bigquery/bigquery_test.gopkg/bigquery/cached_client.gopkg/bigquery/cached_client_test.gopkg/bigquery/fake.gopkg/bigquery/prowjobs.gopkg/bigquery/prowjobs_test.gopkg/cmd/release-payload-controller/cmd.gopkg/cmd/release-payload-controller/jira_escalations_controller.gopkg/cmd/release-payload-controller/jira_escalations_controller_test.gopkg/cmd/release-payload-controller/job_state_controller.gopkg/cmd/release-payload-controller/payload_verification_controller_test.gopkg/cmd/release-payload-controller/qualifiers_summary_controller.gopkg/cmd/release-payload-controller/qualifiers_summary_controller_test.gopkg/cmd/release-payload-controller/release_creation_status_controller_test.gopkg/jira/jira.gopkg/release-controller/types.gopkg/releasepayload/qualifiers/helpers.gopkg/releasepayload/qualifiers/helpers_test.gopkg/releasepayload/v1alpha1helpers/helpers.gopkg/releasepayload/v1alpha1helpers/helpers_test.gopkg/releasequalifiers/config_loader.gopkg/releasequalifiers/config_loader_test.gopkg/releasequalifiers/merge.gopkg/releasequalifiers/merge_test.gopkg/releasequalifiers/notifications/jira/escalation_test.gopkg/releasequalifiers/prettyprint.gopkg/releasequalifiers/prettyprint_test.gopkg/releasequalifiers/types.gopkg/releasequalifiers/validation.gopkg/releasequalifiers/validation_test.go
💤 Files with no reviewable changes (1)
- pkg/release-controller/types.go
✅ Files skipped from review due to trivial changes (1)
- cmd/release-controller-api/static/css/custom.css
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/cmd/release-payload-controller/release_creation_status_controller_test.go
- cmd/release-controller/sync_release_payload.go
- pkg/cmd/release-payload-controller/job_state_controller.go
- pkg/releasepayload/v1alpha1helpers/helpers.go
- pkg/releasequalifiers/types.go
- cmd/release-controller-api/static/releaseStreamPageHtml.tmpl
- pkg/releasequalifiers/validation.go
- cmd/release-controller/sync_release_payload_test.go
- cmd/release-controller-api/static/releasePageHtml.tmpl
- pkg/cmd/release-payload-controller/cmd.go
- cmd/release-controller/config_validate_test.go
- go.mod
- cmd/release-controller-api/http_helper.go
Comment on lines
+1849
to
+1858
| func qualifierStateText(summary v1alpha1.ReleaseQualifierSummary) string { | ||
| if summary.Approval { | ||
| switch summary.AggregateState { | ||
| case v1alpha1.JobStateSuccess: | ||
| return "Accepted" | ||
| case v1alpha1.JobStateFailure: | ||
| return "Rejected" | ||
| default: | ||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Don't render a blank state for pending approval qualifiers.
The default branch under summary.Approval returns "", so any approval qualifier that is still waiting on a decision renders with an empty label here and on the detail page's overall-status badge. Return a stable value like Pending or Awaiting approval instead.
🤖 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/release-controller-api/http.go` around lines 1849 - 1858, The
qualifierStateText function returns an empty string for approval qualifiers in
the default branch, causing pending approvals to render blank; update
qualifierStateText (which inspects summary.Approval and summary.AggregateState)
so that the default case for approval returns a stable label such as "Pending"
or "Awaiting approval" instead of "" (preserve existing returns for
v1alpha1.JobStateSuccess and v1alpha1.JobStateFailure).
Comment on lines
+1953
to
1962
| // Build a lookup of job name -> JobStatus from the payload results | ||
| jobStatusMap := make(map[string]*v1alpha1.JobStatus) | ||
| for i := range payload.Status.BlockingJobResults { | ||
| js := &payload.Status.BlockingJobResults[i] | ||
| jobStatusMap[js.CIConfigurationName] = js | ||
| } | ||
| var approvals string | ||
| if asList { | ||
| approvals += "<ul>" | ||
| for i := range payload.Status.InformingJobResults { | ||
| js := &payload.Status.InformingJobResults[i] | ||
| jobStatusMap[js.CIConfigurationName] = js | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect payload verification fields/results:"
rg -n -C2 'UpgradeJobs|UpgradeJobResults|BlockingJobs|InformingJobs|QualifiersSummary' \
pkg/apis/release/v1alpha1/types.go pkg/releasepayload pkg/cmd/release-payload-controller -g '*.go'
echo
echo "Inspect qualifier collection and API/UI lookup code:"
rg -n -C3 'UpgradeJobs|UpgradeJobResults|CIConfigurationName|Qualifiers' \
cmd/release-controller-api pkg/releasepayload pkg/cmd/release-payload-controller -g '*.go'Repository: openshift/release-controller
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="cmd/release-controller-api/http.go"
echo "== Search for Upgrade-related symbols in $file =="
rg -n 'UpgradeJobs|UpgradeJobResults|UpgradeJob' "$file"
echo
echo "== Show context around lines 1953-1962 in $file =="
nl -ba "$file" | sed -n '1935,1985p'
echo
echo "== Show context around lines 2640-2652 in $file =="
nl -ba "$file" | sed -n '2615,2675p'Repository: openshift/release-controller
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="cmd/release-controller-api/http.go"
echo "== Snippet around 1953-1962 =="
nl -ba "$file" | sed -n '1940,1990p'
echo
echo "== Snippet around 2640-2652 =="
nl -ba "$file" | sed -n '2625,2670p'
echo
echo "== Search for job result fields inside cmd/release-controller-api/http.go =="
rg -n 'BlockingJobResults|InformingJobResults|UpgradeJobResults|UpgradeJobs|UpgradeJob' "$file"
echo
echo "== Search for upgrade job fields anywhere in cmd/release-controller-api =="
rg -n 'UpgradeJobResults|UpgradeJobs|UpgradeJob' cmd/release-controller-api -g '*.go'Repository: openshift/release-controller
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="cmd/release-controller-api/http.go"
echo "== Snippet around 1953-1962 =="
sed -n '1940,1990p' "$file"
echo
echo "== Snippet around 2640-2652 =="
sed -n '2615,2675p' "$file"
echo
echo "== Search for job result fields inside cmd/release-controller-api/http.go =="
rg -n 'BlockingJobResults|InformingJobResults|UpgradeJobResults|UpgradeJobs|UpgradeJob' "$file" || true
echo
echo "== Search for upgrade job fields anywhere in cmd/release-controller-api =="
rg -n 'UpgradeJobResults|UpgradeJobs|UpgradeJob' cmd/release-controller-api -g '*.go' || true
echo
echo "== Search for qualifier lookup construction in cmd/release-controller-api =="
rg -n 'jobStatusMap|QualifiersSummary|CIConfigurationName' cmd/release-controller-api -g '*.go'Repository: openshift/release-controller
Length of output: 7175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find GenerateQualifiersSummary definition =="
rg -n "func (GenerateQualifiersSummary|GenerateQualifiersSummary\\b)" -S pkg || true
rg -n "GenerateQualifiersSummary\\(" -S pkg || true
echo
echo "== Search for UpgradeJobResults usage in qualifier generation =="
rg -n "UpgradeJobResults|UpgradeJobs" -S pkg | head -n 200
echo
echo "== Inspect ReleaseQualifierSummary / QualifiersSummary types =="
rg -n "type (QualifiersSummary|ReleaseQualifierSummary|JobQualifier|JobQualifierRef)" -S pkg/apis/release/v1alpha1/types.go || true
rg -n "QualifiersSummary|ReleaseQualifierSummary" -S pkg/apis/release/v1alpha1/types.go | head -n 200
echo
echo "== Inspect qualifier detail page rendering loop around jobStatusMap usage =="
# show around where http.go references jobStatusMap[jobRef...]
rg -n "jobStatusMap\\[jobRef\\.CIConfigurationName\\]|jobRef\\.CIConfigurationName" cmd/release-controller-api/http.go
sed -n '1985,2105p' cmd/release-controller-api/http.goRepository: openshift/release-controller
Length of output: 14652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect qualifier summary generation to see whether UpgradeJobResults contribute to summary.Jobs
helpers="pkg/releasepayload/qualifiers/helpers.go"
echo "== Relevant sections in $helpers =="
# Show around the loop over status.UpgradeJobResults and any place that iterates over upgrade configs
rg -n "UpgradeJobResults|UpgradeJobs|MultipleJobTypes|status\\.Upgrade|for _, jobStatus := range status\\.UpgradeJobResults|payload\\.Spec\\.PayloadVerificationConfig\\.UpgradeJobs" "$helpers"
echo
echo "== Show code around the UpgradeJobResults loop =="
line=$(rg -n "for _, jobStatus := range status\\.UpgradeJobResults" "$helpers" | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+80))
sed -n "${start},${end}p" "$helpers"
echo
echo "== Show code around building/collecting job refs for qualifiers (summary.Jobs) =="
# heuristically search for "summary.Jobs" or ".Jobs =" patterns
rg -n "summary\\.Jobs|Jobs\\s*:\\s*|\\.Jobs\\s*=" "$helpers"Repository: openshift/release-controller
Length of output: 4838
Fix qualifier detail/config endpoints to include upgrade jobs
GenerateQualifiersSummarybuildssummary.Jobsfrompayload.Spec.PayloadVerificationConfig.UpgradeJobsand indexes results fromstatus.UpgradeJobResults, so upgrade jobs are expected to appear in the qualifier UI.cmd/release-controller-api/http.gobuildsjobStatusMaponly frompayload.Status.BlockingJobResultsandpayload.Status.InformingJobResults, so upgrade jobs render-for status/attempts.- The qualifier config handler searches
payload.Spec.PayloadVerificationConfig.BlockingJobs/InformingJobsonly, so upgrade job rows hit theJob ... not foundpath (404).
🤖 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/release-controller-api/http.go` around lines 1953 - 1962, The job status
lookup and qualifier config search currently omit upgrade jobs; update the
jobStatusMap population in cmd/release-controller-api/http.go to also iterate
payload.Status.UpgradeJobResults (in addition to BlockingJobResults and
InformingJobResults) so upgrade job statuses/attempts are available, and update
the qualifier config handler (the logic that searches
payload.Spec.PayloadVerificationConfig.BlockingJobs and InformingJobs) to also
search payload.Spec.PayloadVerificationConfig.UpgradeJobs (and ensure
GenerateQualifiersSummary uses
payload.Spec.PayloadVerificationConfig.UpgradeJobs when building summary.Jobs)
so upgrade jobs are present in the detail and config endpoints rather than
returning “Job ... not found” or rendering “-”.
Comment on lines
+2638
to
+2673
| // Find the CIConfiguration for the specified job | ||
| var jobConfig *v1alpha1.CIConfiguration | ||
| for i := range payload.Spec.PayloadVerificationConfig.BlockingJobs { | ||
| if payload.Spec.PayloadVerificationConfig.BlockingJobs[i].CIConfigurationName == jobName { | ||
| jobConfig = &payload.Spec.PayloadVerificationConfig.BlockingJobs[i] | ||
| break | ||
| } | ||
| } | ||
| if jobConfig == nil { | ||
| for i := range payload.Spec.PayloadVerificationConfig.InformingJobs { | ||
| if payload.Spec.PayloadVerificationConfig.InformingJobs[i].CIConfigurationName == jobName { | ||
| jobConfig = &payload.Spec.PayloadVerificationConfig.InformingJobs[i] | ||
| break | ||
| } | ||
| } | ||
| } | ||
| if jobConfig == nil { | ||
| http.Error(w, fmt.Sprintf("Job %q not found in payload %q", jobName, payload.Name), http.StatusNotFound) | ||
| return | ||
| } | ||
|
|
||
| // Start with the global config as the base | ||
| var mergedQualifier releasequalifiers.ReleaseQualifier | ||
| if c.configAccessor != nil { | ||
| globalConfig := c.configAccessor.Get() | ||
| if globalConfig != nil { | ||
| if gq, ok := globalConfig[qualifierID]; ok { | ||
| mergedQualifier = gq | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Merge per-job override on top | ||
| if override, ok := jobConfig.Qualifiers[qualifierID]; ok { | ||
| mergedQualifier = mergedQualifier.Merge(override) | ||
| } |
Contributor
There was a problem hiding this comment.
Reject jobs that do not actually reference this qualifier.
Once jobConfig is found, this handler merges and returns the global qualifier config even when jobConfig.Qualifiers does not contain qualifierID. That makes /qualifier/{id}/config?job=... return a misleading config for unrelated jobs instead of a 404.
Proposed fix
// Find the CIConfiguration for the specified job
var jobConfig *v1alpha1.CIConfiguration
for i := range payload.Spec.PayloadVerificationConfig.BlockingJobs {
if payload.Spec.PayloadVerificationConfig.BlockingJobs[i].CIConfigurationName == jobName {
jobConfig = &payload.Spec.PayloadVerificationConfig.BlockingJobs[i]
break
}
}
if jobConfig == nil {
for i := range payload.Spec.PayloadVerificationConfig.InformingJobs {
if payload.Spec.PayloadVerificationConfig.InformingJobs[i].CIConfigurationName == jobName {
jobConfig = &payload.Spec.PayloadVerificationConfig.InformingJobs[i]
break
}
}
}
if jobConfig == nil {
http.Error(w, fmt.Sprintf("Job %q not found in payload %q", jobName, payload.Name), http.StatusNotFound)
return
}
+ if _, ok := jobConfig.Qualifiers[qualifierID]; !ok {
+ http.Error(w, fmt.Sprintf("Qualifier %q is not configured for job %q", qualifierID, jobName), http.StatusNotFound)
+ return
+ }Based on merge semantics in pkg/releasequalifiers/merge.go, merging only defines override precedence; it does not establish that a job participates in the qualifier.
🤖 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/release-controller-api/http.go` around lines 2638 - 2673, After locating
jobConfig, explicitly verify that the job actually references the qualifier by
checking jobConfig.Qualifiers for qualifierID and return a 404 if it is not
present; do this before using the global config. In other words, after finding
jobConfig (symbol: jobConfig) add a presence check for qualifierID in
jobConfig.Qualifiers (use the map lookup pattern), and if not found call
http.Error(...) with StatusNotFound and return; only then proceed to build
mergedQualifier by loading the global config via c.configAccessor.Get(),
applying the global base (gq) and then Merge the job override
(mergedQualifier.Merge) from jobConfig.Qualifiers[qualifierID].
Comment on lines
+176
to
+215
| collectFromJobs := func(jobs []v1alpha1.CIConfiguration) { | ||
| for _, job := range jobs { | ||
| if len(job.Qualifiers) == 0 || seen[job.CIConfigurationJobName] { | ||
| continue | ||
| } | ||
|
|
||
| for qualifierID := range job.Qualifiers { | ||
| globalConfig, exists := qualifiersConfig[qualifierID] | ||
| if !exists { | ||
| continue | ||
| } | ||
|
|
||
| jobQualifierConfig := job.Qualifiers[qualifierID] | ||
| merged := globalConfig.Merge(jobQualifierConfig) | ||
|
|
||
| if merged.Notifications == nil || merged.Notifications.Jira == nil || len(merged.Notifications.Jira.Escalations) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| seen[job.CIConfigurationJobName] = true | ||
|
|
||
| // Use the last escalation's OverPeriod to determine the query interval | ||
| lastEscalation := merged.Notifications.Jira.Escalations[len(merged.Notifications.Jira.Escalations)-1] | ||
| if lastEscalation.OverPeriod != "" { | ||
| interval, err := parsePeriodToSQLInterval(lastEscalation.OverPeriod) | ||
| if err != nil { | ||
| klog.Warningf("Invalid OverPeriod %q for job %s, using default 14-day window: %v", | ||
| lastEscalation.OverPeriod, job.CIConfigurationJobName, err) | ||
| defaultJobs = append(defaultJobs, job.CIConfigurationJobName) | ||
| } else { | ||
| filteredJobs = append(filteredJobs, bigquery.ProwjobQueryFilter{ | ||
| Name: job.CIConfigurationJobName, | ||
| Interval: interval, | ||
| }) | ||
| } | ||
| } else { | ||
| defaultJobs = append(defaultJobs, job.CIConfigurationJobName) | ||
| } | ||
| break | ||
| } |
Contributor
There was a problem hiding this comment.
Make the BigQuery window deterministic per job.
job.Qualifiers is a Go map, so this loop picks whichever Jira-enabled qualifier happens to be visited first, marks the job as seen, and breaks. If one job carries multiple Jira-enabled qualifiers with different OverPeriod settings, the queried history window becomes nondeterministic and can be too short for the other qualifier(s), which will miss escalations.
Please compute a single deterministic superset window for the job before setting seen—for example, the largest required interval across all Jira-enabled qualifiers for that job.
🤖 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 `@pkg/cmd/release-payload-controller/jira_escalations_controller.go` around
lines 176 - 215, In collectFromJobs, job.Qualifiers is iterated
nondeterministically and you mark seen and break on the first Jira-enabled
qualifier found; instead compute a deterministic superset interval for the job
by scanning all qualifierIDs for that job (using qualifiersConfig and
job.Qualifiers), merging each qualifier (globalConfig.Merge(jobQualifierConfig))
and collecting all non-empty merged.Notifications.Jira.Escalations OverPeriod
values, converting each with parsePeriodToSQLInterval and taking the largest
interval (superset) before setting seen[job.CIConfigurationJobName]; then push
either a single bigquery.ProwjobQueryFilter with that superset interval onto
filteredJobs or fall back to defaultJobs if none valid, and ensure
signing/logging of parse errors per-qualifier while continuing the superset
computation.
Comment on lines
+289
to
+308
| if highestEscalation != nil { | ||
| // Duplicate prevention: skip if same escalation already active and not abated | ||
| if notifState.ActiveEscalation == highestEscalation.Name && | ||
| notifState.ActivePriority == highestEscalation.Priority && | ||
| !notifState.Abated { | ||
| klog.V(4).Infof("Skipping duplicate escalation '%s' for thread %s", highestEscalation.Name, threadID) | ||
| continue | ||
| } | ||
|
|
||
| issueKey := c.triggerJiraEscalation(ctx, releasePayload, job, qualifierID, jiraConfig, *highestEscalation) | ||
|
|
||
| // Update notification state | ||
| notifState.ActiveEscalation = highestEscalation.Name | ||
| notifState.ActivePriority = highestEscalation.Priority | ||
| notifState.Abated = false | ||
| notifState.LastTransitionTime = metav1.NewTime(time.Now()) | ||
| if issueKey != "" { | ||
| notifState.IssueKey = issueKey | ||
| } | ||
| c.setNotificationState(releasePayload, qualifierID, threadID, notifState) |
Contributor
There was a problem hiding this comment.
Don't mark the escalation active when Jira creation/update failed.
triggerJiraEscalation() returns "" when Jira is unavailable or the search/create/update path fails, but this block still persists ActiveEscalation, ActivePriority, Abated=false, and a transition time. On the next sync, the duplicate check treats that escalation as already active and suppresses retries, so a transient Jira outage can permanently drop the notification until conditions abate.
Suggested direction
- issueKey := c.triggerJiraEscalation(ctx, releasePayload, job, qualifierID, jiraConfig, *highestEscalation)
-
- // Update notification state
- notifState.ActiveEscalation = highestEscalation.Name
- notifState.ActivePriority = highestEscalation.Priority
- notifState.Abated = false
- notifState.LastTransitionTime = metav1.NewTime(time.Now())
- if issueKey != "" {
- notifState.IssueKey = issueKey
- }
- c.setNotificationState(releasePayload, qualifierID, threadID, notifState)
+ issueKey := c.triggerJiraEscalation(ctx, releasePayload, job, qualifierID, jiraConfig, *highestEscalation)
+ if issueKey == "" {
+ continue
+ }
+
+ notifState.ActiveEscalation = highestEscalation.Name
+ notifState.ActivePriority = highestEscalation.Priority
+ notifState.Abated = false
+ notifState.LastTransitionTime = metav1.NewTime(time.Now())
+ notifState.IssueKey = issueKey
+ c.setNotificationState(releasePayload, qualifierID, threadID, notifState)Returning an error from triggerJiraEscalation() and letting the controller requeue would be even safer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if highestEscalation != nil { | |
| // Duplicate prevention: skip if same escalation already active and not abated | |
| if notifState.ActiveEscalation == highestEscalation.Name && | |
| notifState.ActivePriority == highestEscalation.Priority && | |
| !notifState.Abated { | |
| klog.V(4).Infof("Skipping duplicate escalation '%s' for thread %s", highestEscalation.Name, threadID) | |
| continue | |
| } | |
| issueKey := c.triggerJiraEscalation(ctx, releasePayload, job, qualifierID, jiraConfig, *highestEscalation) | |
| // Update notification state | |
| notifState.ActiveEscalation = highestEscalation.Name | |
| notifState.ActivePriority = highestEscalation.Priority | |
| notifState.Abated = false | |
| notifState.LastTransitionTime = metav1.NewTime(time.Now()) | |
| if issueKey != "" { | |
| notifState.IssueKey = issueKey | |
| } | |
| c.setNotificationState(releasePayload, qualifierID, threadID, notifState) | |
| if highestEscalation != nil { | |
| // Duplicate prevention: skip if same escalation already active and not abated | |
| if notifState.ActiveEscalation == highestEscalation.Name && | |
| notifState.ActivePriority == highestEscalation.Priority && | |
| !notifState.Abated { | |
| klog.V(4).Infof("Skipping duplicate escalation '%s' for thread %s", highestEscalation.Name, threadID) | |
| continue | |
| } | |
| issueKey := c.triggerJiraEscalation(ctx, releasePayload, job, qualifierID, jiraConfig, *highestEscalation) | |
| if issueKey == "" { | |
| continue | |
| } | |
| notifState.ActiveEscalation = highestEscalation.Name | |
| notifState.ActivePriority = highestEscalation.Priority | |
| notifState.Abated = false | |
| notifState.LastTransitionTime = metav1.NewTime(time.Now()) | |
| notifState.IssueKey = issueKey | |
| c.setNotificationState(releasePayload, qualifierID, threadID, notifState) |
🤖 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 `@pkg/cmd/release-payload-controller/jira_escalations_controller.go` around
lines 289 - 308, The code marks an escalation active even when
triggerJiraEscalation(...) failed (returns empty issueKey), causing retries to
be suppressed; change the logic in the block that handles highestEscalation so
that you only set notifState.ActiveEscalation, notifState.ActivePriority,
notifState.Abated=false, notifState.LastTransitionTime and call
c.setNotificationState(...) when triggerJiraEscalation(...) returns a non-empty
issueKey (or propagate an error from triggerJiraEscalation to requeue the
reconcile); specifically, check issueKey before mutating notifState and
persisting via setNotificationState, keeping the duplicate-suppression behavior
only for successfully-created/updated Jira issues.
Comment on lines
246
to
250
| for _, comment := range issue.Fields.Comments.Comments { | ||
| // if a ticket is on the verified state but does not contain a comment from the bot, it will add one | ||
| // if a manually verified ticket is already commented, we won't check the message body | ||
| if (comment.Body == message || strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) && (comment.Author.Name == "openshift-crt-jira-release-controller" || comment.Author.EmailAddress == "brawilli+openshift-crt-jira-release-controller@redhat.com" || comment.Author.EmailAddress == "brawilli@redhat.com" || comment.Author.EmailAddress == "openshift-release-controller-jira-bot@redhat.com") { | ||
| if (comment.Body == message || strings.EqualFold(issue.Fields.Status.Name, jira.StatusVerified)) && (comment.Author.Name == "openshift-crt-jira-release-controller" || comment.Author.EmailAddress == "brawilli+openshift-crt-jira-release-controller@redhat.com" || comment.Author.EmailAddress == "brawilli@redhat.com") { | ||
| return |
Contributor
There was a problem hiding this comment.
Keep the legacy bot email in the dedup check.
This predicate no longer matches comments authored as openshift-release-controller-jira-bot@redhat.com, so older verified issues that already have a bot comment under that identity will get a duplicate verification comment the next time they are processed.
Restoring that legacy address here keeps the change backward-compatible with existing Jira history.
🤖 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 `@pkg/jira/jira.go` around lines 246 - 250, The deduplication predicate inside
the loop over issue.Fields.Comments.Comments is missing the legacy bot email and
therefore can create duplicate verification comments; update the condition that
checks comment.Author.EmailAddress (alongside comment.Author.Name and
comment.Body and issue.Fields.Status.Name/jira.StatusVerified) to also accept
"openshift-release-controller-jira-bot@redhat.com" in the OR list so older
comments by that legacy identity are treated as bot comments and cause an early
return.
Contributor
|
@bradmwilliams: 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. |
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
1 participant
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.
This PR contains the Release Qualifying Jobs implementation, based on the work in #771.
rh-pre-commit.version: 2.4.0
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit
New Features
Style