Repository navigation
Enable possibility to use capabilities in images jobs - #4964
Merged
openshift-merge-bot[bot] merged 1 commit intoMay 7, 2026
Merged
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
📝 WalkthroughWalkthroughAdds a new ChangesCapability-driven multi-arch and labeling
Sequence Diagram(s)sequenceDiagram
participant Author as Config Author
participant API as pkg/api (types)
participant Validator as pkg/validation
participant Prowgen as pkg/prowgen
participant Operator as pkg/image-graph-generator / pkg/steps
Author->>API: define image build (Capabilities, AdditionalArchitectures)
API->>Validator: provide config for validation
Validator->>API: validate Capabilities against ValidArchitectures / qualified-name rules
API->>Prowgen: expose AllCapabilities() / ArchitectureCapabilities()
Prowgen->>Prowgen: generate jobs, add labels capability/<cap> for each AllCapabilities()
Prowgen->>Operator: include capability/architecture metadata in produced job info
Operator->>Operator: use ArchitectureCapabilities() to determine multi-arch behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
jmguzik
force-pushed
the
cherry-pick-approved
branch
from
February 25, 2026 09:57
68b738a to
87ba8c5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/validation/config.go (1)
326-333: Capability validation error message lacks guidance for non-architecture values.When a capability fails
IsQualifiedName, the error only says"invalid capability: <cap>"without describing what format is expected (unlike the architecture error at Line 323 which lists valid values). Users won't know to use a qualified name format.📝 Proposed improvement
if err := validation.IsQualifiedName(cap); len(err) > 0 { - validationErrors = append(validationErrors, ctxN.errorf("invalid capability: %s", cap)) + validationErrors = append(validationErrors, ctxN.errorf("invalid capability %q: must be a valid architecture (%s) or a qualified name (e.g. 'org.io/feature')", cap, strings.Join(sets.List(api.ValidArchitectures), ", "))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/validation/config.go` around lines 326 - 333, The capability validation loop over image.Capabilities currently reports "invalid capability: %s" when validation.IsQualifiedName(cap) fails; update this to provide guidance like "capability must be a qualified name (DNS subdomain format), e.g. <domain>/<name>: %s" so users know the expected format; modify the error produced by ctxN.errorf in that branch (near the loop that checks api.ValidArchitectures and calls validation.IsQualifiedName) to include this explanatory text or a short reference to "qualified name" / "DNS subdomain" format.pkg/api/types.go (1)
2853-2855:appendaliasing inAllCapabilities()is safe but worth noting.
append(p.Capabilities, p.AdditionalArchitectures...)could mutatep.Capabilities's underlying array if it has spare capacity. The result is immediately consumed bysets.New[string](...), so no observable aliasing occurs here, but it's a subtle pattern. An explicit copy orslices.Concatwould be safer if the method is ever reused elsewhere.♻️ Safer alternative
func (p *ProjectDirectoryImageBuildStepConfiguration) AllCapabilities() []string { - return sets.List(sets.New[string](append(p.Capabilities, p.AdditionalArchitectures...)...)) + combined := make([]string, 0, len(p.Capabilities)+len(p.AdditionalArchitectures)) + combined = append(combined, p.Capabilities...) + combined = append(combined, p.AdditionalArchitectures...) + return sets.List(sets.New[string](combined...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/types.go` around lines 2853 - 2855, The current AllCapabilities method uses append(p.Capabilities, p.AdditionalArchitectures...) which can mutate p.Capabilities' underlying array; to avoid aliasing, create a fresh slice before combining (e.g., allocate a new slice of len(p.Capabilities)+len(p.AdditionalArchitectures) and copy p.Capabilities and p.AdditionalArchitectures into it) and pass that new slice to sets.New[string](...) so ProjectDirectoryImageBuildStepConfiguration.AllCapabilities returns a set list without mutating p.Capabilities; alternatively use slices.Concat/slices.Clone to produce a non-aliased combined slice prior to calling sets.New.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_merge_capabilities_and_architecture_labels.yaml`:
- Around line 1-17: Test fixtures use the incorrect architecture string
"ppc64-le" (with a hyphen) which doesn't match ValidArchitectures (contains
"ppc64le"), causing injectCapabilitiesForImgJobs to add labels like
capability/ppc64-le while ResolveMultiArch and ArchitectureCapabilities filter
it out; fix the fixtures by replacing all occurrences of "ppc64-le" with
"ppc64le" (e.g., in test data and fixtures used by the test that passes
AdditionalArchitectures: []string{"ppc64-le"}) so ValidArchitectures.Has(...)
will succeed and ResolveMultiArch, ArchitectureCapabilities and AllCapabilities
behave consistently with injectCapabilitiesForImgJobs.
---
Nitpick comments:
In `@pkg/api/types.go`:
- Around line 2853-2855: The current AllCapabilities method uses
append(p.Capabilities, p.AdditionalArchitectures...) which can mutate
p.Capabilities' underlying array; to avoid aliasing, create a fresh slice before
combining (e.g., allocate a new slice of
len(p.Capabilities)+len(p.AdditionalArchitectures) and copy p.Capabilities and
p.AdditionalArchitectures into it) and pass that new slice to
sets.New[string](...) so
ProjectDirectoryImageBuildStepConfiguration.AllCapabilities returns a set list
without mutating p.Capabilities; alternatively use slices.Concat/slices.Clone to
produce a non-aliased combined slice prior to calling sets.New.
In `@pkg/validation/config.go`:
- Around line 326-333: The capability validation loop over image.Capabilities
currently reports "invalid capability: %s" when validation.IsQualifiedName(cap)
fails; update this to provide guidance like "capability must be a qualified name
(DNS subdomain format), e.g. <domain>/<name>: %s" so users know the expected
format; modify the error produced by ctxN.errorf in that branch (near the loop
that checks api.ValidArchitectures and calls validation.IsQualifiedName) to
include this explanatory text or a short reference to "qualified name" / "DNS
subdomain" format.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (8)
pkg/api/types.gopkg/image-graph-generator/operator.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_merge_capabilities_and_architecture_labels.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_multiarch_postsubmit_images__using_capabilities.yamlpkg/steps/project_image.gopkg/validation/config.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/api/types.go (1)
2853-2855: Prefer copy-first before append inAllCapabilities()to ensurep.Capabilitiesis never modified by the append operation.Line 2854 appends directly to
p.Capabilities, which can modify its backing array if it has capacity. For a public API method, copy-first is safer defensive programming and avoids subtle backing-array mutations.♻️ Suggested refactor
func (p *ProjectDirectoryImageBuildStepConfiguration) AllCapabilities() []string { - return sets.List(sets.New[string](append(p.Capabilities, p.AdditionalArchitectures...)...)) + caps := append([]string{}, p.Capabilities...) + caps = append(caps, p.AdditionalArchitectures...) + return sets.List(sets.New[string](caps...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/types.go` around lines 2853 - 2855, The AllCapabilities method mutates p.Capabilities by appending p.AdditionalArchitectures directly; fix it by making a copy of p.Capabilities first, append AdditionalArchitectures to that copy, then pass the combined slice into sets.New/sets.List. Update ProjectDirectoryImageBuildStepConfiguration.AllCapabilities to create a new slice (copy of p.Capabilities), do append(copy, p.AdditionalArchitectures...), and use sets.New[string](combined...) so the original p.Capabilities backing array is never modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 745-746: Update the test inputs that use the non-canonical
architecture token "ppc64-le" to the canonical "ppc64le" so capability-based
filtering matches the production code; specifically, locate the Capabilities
slices in the test cases (the Capabilities: []string{"arm64", "ppc64-le"}
entries in prowgen_test.go) and replace "ppc64-le" with "ppc64le" in both
occurrences (the block around the first mention and the block around the later
mention at lines referenced in the review).
In `@pkg/validation/config.go`:
- Around line 326-332: The capability validation currently lets values with
slashes through because it only checks validation.IsQualifiedName; update the
loop over image.Capabilities to also reject values that are not valid Kubernetes
label values by invoking validation.IsValidLabelValue(cap) and appending
ctxN.errorf("invalid capability: %s", cap) when that check fails (in addition to
the existing api.ValidArchitectures and IsQualifiedName checks); this ensures
the values used later in prowgen (labels[fmt.Sprintf("capability/%s", c)] = c)
conform to label value requirements.
---
Nitpick comments:
In `@pkg/api/types.go`:
- Around line 2853-2855: The AllCapabilities method mutates p.Capabilities by
appending p.AdditionalArchitectures directly; fix it by making a copy of
p.Capabilities first, append AdditionalArchitectures to that copy, then pass the
combined slice into sets.New/sets.List. Update
ProjectDirectoryImageBuildStepConfiguration.AllCapabilities to create a new
slice (copy of p.Capabilities), do append(copy, p.AdditionalArchitectures...),
and use sets.New[string](combined...) so the original p.Capabilities backing
array is never modified.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (8)
pkg/api/types.gopkg/image-graph-generator/operator.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_merge_capabilities_and_architecture_labels.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_multiarch_postsubmit_images__using_capabilities.yamlpkg/steps/project_image.gopkg/validation/config.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/steps/project_image.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_merge_capabilities_and_architecture_labels.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_multiarch_postsubmit_images__using_capabilities.yaml
jmguzik
force-pushed
the
cherry-pick-approved
branch
from
February 25, 2026 11:12
87ba8c5 to
ef57abd
Compare
Contributor
|
/lgtm |
Member
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
droslean
reviewed
Feb 25, 2026
|
|
||
| func injectArchitectureLabels(g *prowJobBaseBuilder, imagesConfig []cioperatorapi.ProjectDirectoryImageBuildStepConfiguration) { | ||
| for _, imageConfig := range imagesConfig { | ||
| for _, arch := range imageConfig.AdditionalArchitectures { |
Member
There was a problem hiding this comment.
Will this change break the current behavior?
Contributor
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
Author
|
/retest |
Make capabilities and additional_architectures equivalent during transition. Both fields are merged into a unified capabilities list via AllCapabilities(). For architecture-specific logic (e.g. ResolveMultiArch), ArchitectureCapabilities() filters the merged list against valid architecture strings. - Add AllCapabilities() and ArchitectureCapabilities() helpers - Mark AdditionalArchitectures as deprecated in favor of Capabilities - Update prowgen, validation, project_image, and image-graph-generator to use the new helpers instead of accessing fields directly - Add validation for Capabilities values - Fix Capabilities comment on image build config (was "test runs") Made with Cursor Signed-off-by: Jakub Guzik <jguzik@redhat.com>
jmguzik
force-pushed
the
cherry-pick-approved
branch
from
May 6, 2026 10:29
ef57abd to
6f19cfa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/types.go (1)
2793-2817: Coordinate downstream vendoring for the API type change.Since
ProjectDirectoryImageBuildStepConfigurationinpkg/apigainedCapabilities, plan follow-up vendor bumps in known external consumers (e.g., release-controller, ci-chat-bot) to avoid prolonged schema drift.🤖 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/api/types.go` around lines 2793 - 2817, ProjectDirectoryImageBuildStepConfiguration gained a new field Capabilities causing downstream schema drift; update downstream vendors. Locate the API type ProjectDirectoryImageBuildStepConfiguration in pkg/api (and references to the Capabilities field) and coordinate bumping the api version in downstream consumers (e.g., release-controller, ci-chat-bot and any other repos that import pkg/api) by updating their vendor/module dependencies to the new commit/tag that includes this change; run their build/tests to catch compile/schema failures and fix any call sites that assume the older struct (add handling for Capabilities or default behavior) before merging.
🤖 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.
Nitpick comments:
In `@pkg/api/types.go`:
- Around line 2793-2817: ProjectDirectoryImageBuildStepConfiguration gained a
new field Capabilities causing downstream schema drift; update downstream
vendors. Locate the API type ProjectDirectoryImageBuildStepConfiguration in
pkg/api (and references to the Capabilities field) and coordinate bumping the
api version in downstream consumers (e.g., release-controller, ci-chat-bot and
any other repos that import pkg/api) by updating their vendor/module
dependencies to the new commit/tag that includes this change; run their
build/tests to catch compile/schema failures and fix any call sites that assume
the older struct (add handling for Capabilities or default behavior) before
merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 32618165-2a7c-4d3b-b069-826ee0bc8a06
⛔ Files ignored due to path filters (1)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
pkg/api/types.gopkg/image-graph-generator/operator.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_merge_capabilities_and_architecture_labels.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_multiarch_postsubmit_images.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_multiarch_postsubmit_images__using_capabilities.yaml
Contributor
|
Scheduling tests matching the |
Contributor
|
/lgtm |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007, droslean, hector-vido, jmguzik 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 |
Contributor
Contributor
|
/override ci/prow/images |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
Contributor
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/images 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. |
openshift-merge-bot
Bot
merged commit May 7, 2026
81f171f
into
openshift:main
16 of 17 checks passed
Contributor
|
@jmguzik: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make capabilities and additional_architectures equivalent during transition. Both fields are merged into a unified capabilities list via AllCapabilities(). For architecture-specific logic (e.g. ResolveMultiArch), ArchitectureCapabilities() filters the merged list against valid architecture strings.
Summary by CodeRabbit
New Features
Deprecations
CI / Tests
Validation