OCPSTRAT-2876: Allow inclusion/exclusion of manifests based on the OCP major version#1282
OCPSTRAT-2876: Allow inclusion/exclusion of manifests based on the OCP major version#1282JoelSpeed wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRelease semver is parsed and the major version is threaded through payload render and CVO flows; manifest inclusion/filtering now considers Changes
Sequence DiagramsequenceDiagram
participant Release as Release Metadata
participant Parser as Version Parser
participant Payload as Payload/Render
participant CVO as CVO Sync Worker
participant Filter as Manifest Include/Filter
participant Cluster as Cluster API
Release->>Parser: loadReleaseVersion()
Parser->>Parser: Parse semantic version (e.g. 5.0.0)
Parser->>Payload: provide ParsedVersion.Major
Payload->>CVO: pass majorVersion (*uint64)
CVO->>Filter: GetImplicitlyEnabledCapabilities(manifests, ..., majorVersion)
CVO->>Filter: Include(manifests..., majorVersion)
Filter->>Filter: Evaluate `release.openshift.io/major-version` annotation against majorVersion
alt matches major
Filter->>Cluster: apply manifest
else excluded
Filter-->>CVO: skip manifest
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
JoelSpeed
force-pushed
the
major-version-inclusion
branch
2 times, most recently
from
39354c8 to
ef0e8c6
Compare
JoelSpeed
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
ef0e8c6 to
2d6aae6
Compare
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
JoelSpeed
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@DavidHurta If you are available, I'd appreciate if you could help me with this feature as well |
|
@JoelSpeed, I will be happy to assist. /cc |
DavidHurta
left a comment
DavidHurta
left a comment
There was a problem hiding this comment.
The PR looks solid!
A few comments about a thing here or there, but nothing drastic.
The overall functionality is 👍
pkg/payload/payload.go
Outdated
| ImageRef *imagev1.ImageStream | ||
|
|
||
| Architecture string | ||
| MajorVersion *uint64 |
There was a problem hiding this comment.
Here, the purpose of MajorVersion is starting to become a bit fuzzy in comparison to the Release.Version field. The meaning of MajorVersion in the inclusion filtering context is clear; however, here we use it in a general struct type representing the contents of a release image. Thus, their purpose becomes less clear.
We could do one of the following:
- Document
MajorVersionfield to make the difference betweenRelease.VersionandMajorVersionclearer. - Leave only one source of truth (that is,
Release.Version), and extract the major version when needed using a function. - Create a new field
ParsedVersion semver.Version; we could use it to get access to the parsed cached version of the update easily. This could even be utilized in the future when we need easy access to the parsed version of an update, which is more handy for fine grained processing. This would bring additional value to the new field, make it more general, and make its distinction clearer. The call sites requiring theMajorVersionwould need to be updated to justptr.To(payload.ParsedVersion.Major), so it would still be readable. I am more inclined towards this approach, but I am curious what you think.
There was a problem hiding this comment.
I like the parsed version approach, I'll see what that looks like locally
| return requiredFeatureSet, enabledFeatureGates, nil | ||
| } | ||
|
|
||
| func loadReleaseVersion(releaseDir string) (semver.Version, error) { |
There was a problem hiding this comment.
Here we start to duplicate some logic. I am curious whether we could leverage the existing loadReleaseMetadata function here instead of manually loading the release version.
The loadReleaseMetadata function also contains some additional validation, such as:
if metadata.Kind != "cincinnati-metadata-v0" {
return release, fmt.Errorf("unrecognized Cincinnati metadata kind %q", metadata.Kind)
}There was a problem hiding this comment.
Something like:
func loadReleaseVersion(releaseDir string) (semver.Version, error) {
release, err := loadReleaseMetadata(releaseDir)
if err != nil {
return semver.Version{}, err
}
parsedVersion, err := semver.Parse(release.Version)
if err != nil {
return semver.Version{}, err
}
return parsedVersion, nil
}| } | ||
| } | ||
|
|
||
| func TestRenderDirWithMajorVersionFiltering(t *testing.T) { |
There was a problem hiding this comment.
Comment not related to the selected line
Thank you for the complex testing of the rendering!
The overall testing coverage of the PR is solid. However, we do not have an integration test to determine whether the CVO will actually apply the manifest based on the major version. I am doubtful whether this feature will receive end-to-end tests. Thus, I think it would be beneficial to increase the test coverage to ensure that the CVO truly does respect the major version when loading and applying manifests, WDYT?
Something like: DavidHurta@ee1907b
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
2d6aae6 to
adefaa3
Compare
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
adefaa3 to
ede695d
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 37: Remove the personal-fork replace directive that points "replace
github.com/openshift/library-go => github.com/JoelSpeed/library-go
v0.0.0-20260130121715-6e5ccfd0da42" from go.mod and instead reference the
official upstream module; update the module requirement for
github.com/openshift/library-go to the appropriate upstream version/tag (or
remove the replace entirely if the required upstream version is already
satisfied) so builds use the official org package rather than the JoelSpeed
fork.
In `@lib/manifest/manifest.go`:
- Line 67: The test suite lacks coverage for cross-major-version behavior in
GetImplicitlyEnabledCapabilities: add a new case within
TestGetImplicitlyEnabledCapabilities that constructs a currentManifest and an
updateManifest with different MajorVersion values (e.g., currentMajor=4,
updateMajor=5) and invokes the code path that uses
manifestInclusionConfiguration.MajorVersion for each manifest evaluation; ensure
the updateManifest is evaluated with the update payload's MajorVersion (set the
manifestInclusionConfiguration used for updateManifest accordingly) and assert
the returned implicitly enabled capabilities reflect the update-major evaluation
rather than the current-major one, referencing
TestGetImplicitlyEnabledCapabilities, GetImplicitlyEnabledCapabilities,
currentManifest, updateManifest, and manifestInclusionConfiguration when
locating where to add the case.
In `@pkg/cvo/sync_worker.go`:
- Around line 451-452: The implicit-capability calculation is using the current
payload major version (w.payload.ParsedVersion.Major) instead of the
target/update payload major version, causing incorrect results during
cross-major upgrades; update the call to
payload.GetImplicitlyEnabledCapabilities (the invocation that assigns
implicitlyEnabledCaps) to pass the target payload's major version
(payloadUpdate.ParsedVersion.Major) via ptr.To(...) rather than
w.payload.ParsedVersion.Major so the evaluation uses the update payload's
version.
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml`:
- Around line 4-11: The CRD's metadata.name is invalid — update metadata.name to
follow the required "<plural>.<group>" format by replacing the current value of
metadata.name with the concatenation of spec.names.plural and spec.group (e.g.,
set metadata.name to "noversions.test.io") so it matches spec.names.plural and
spec.group; verify the fields metadata.name, spec.names.plural, and spec.group
are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86b819d9-2e3c-4b03-9cde-3e35027f9e1a
⛔ Files ignored due to path filters (21)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/manifest/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modlib/manifest/manifest.gopkg/cvo/cvo_scenarios_test.gopkg/cvo/featuregate_integration_test.gopkg/cvo/sync_worker.gopkg/cvo/testdata/majorversiontest-v5/manifests/.gitkeeppkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_40_version-4-or-5.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/image-referencespkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadatapkg/payload/payload.gopkg/payload/payload_test.gopkg/payload/render.gopkg/payload/render_test.go
| name: noversion | ||
| annotations: | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| spec: | ||
| group: test.io | ||
| names: | ||
| kind: NoVersion | ||
| plural: noversions |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes CustomResourceDefinition metadata.name format spec.names.plural spec.group
💡 Result:
For a Kubernetes CustomResourceDefinition (CRD), the required relationship is:
metadata.nameMUST be:
<spec.names.plural>.<spec.group>(v1-32.docs.kubernetes.io)
So, given:
spec.group: stable.example.comspec.names.plural: crontabs
Then:
metadata.name: crontabs.stable.example.com(v1-32.docs.kubernetes.io)
Additional relevant constraints:
spec.names.pluralmust be all lowercase. (kubernetes.io)
Citations:
- 1: https://v1-32.docs.kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/?utm_source=openai
- 2: https://v1-32.docs.kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/?utm_source=openai
- 3: https://kubernetes.io/zh-cn/docs/reference/kubernetes-api/extend-resources/custom-resource-definition-v1?utm_source=openai
Use a valid CRD name format (<plural>.<group>)
Line 4 sets metadata.name: noversion, but with spec.names.plural: noversions and spec.group: test.io, this must be noversions.test.io per the Kubernetes CRD API contract. Invalid CRD names violate the required naming constraint and will fail validation.
Suggested fix
metadata:
- name: noversion
+ name: noversions.test.io
annotations:
include.release.openshift.io/self-managed-high-availability: "true"📝 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.
| name: noversion | |
| annotations: | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| spec: | |
| group: test.io | |
| names: | |
| kind: NoVersion | |
| plural: noversions | |
| name: noversions.test.io | |
| annotations: | |
| include.release.openshift.io/self-managed-high-availability: "true" | |
| spec: | |
| group: test.io | |
| names: | |
| kind: NoVersion | |
| plural: noversions |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml`
around lines 4 - 11, The CRD's metadata.name is invalid — update metadata.name
to follow the required "<plural>.<group>" format by replacing the current value
of metadata.name with the concatenation of spec.names.plural and spec.group
(e.g., set metadata.name to "noversions.test.io") so it matches
spec.names.plural and spec.group; verify the fields metadata.name,
spec.names.plural, and spec.group are consistent.
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
ede695d to
47b4565
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml (1)
4-11:⚠️ Potential issue | 🟠 MajorCRD name mismatch remains unresolved (
metadata.namevs<plural>.<group>).This still should be
noversions.test.ioto satisfy CRD naming requirements.Suggested fix
metadata: - name: noversion + name: noversions.test.io🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml` around lines 4 - 11, The CRD's metadata.name is invalidly set to "noversion"; update metadata.name to match the required <plural>.<group> pattern by setting it to "noversions.test.io" so it matches spec.names.plural ("noversions") and spec.group ("test.io")—look for the metadata.name field and the spec.names.plural / spec.group entries in this manifest (the resource currently named "noversion") and change metadata.name to "noversions.test.io".lib/manifest/manifest.go (1)
60-87:⚠️ Potential issue | 🟠 MajorSeparate major-version context for update vs current manifest evaluation.
Both branches currently use the same
MajorVersion. In cross-major upgrades,currentManifestshould be evaluated with the current payload major, whileupdateManifestshould use the target payload major.Possible fix direction
-func GetImplicitlyEnabledCapabilities( - updatePayloadManifests []manifest.Manifest, - currentPayloadManifests []manifest.Manifest, - manifestInclusionConfiguration InclusionConfiguration, - currentImplicitlyEnabled sets.Set[configv1.ClusterVersionCapability], -) sets.Set[configv1.ClusterVersionCapability] { +func GetImplicitlyEnabledCapabilities( + updatePayloadManifests []manifest.Manifest, + currentPayloadManifests []manifest.Manifest, + updateInclusionConfiguration InclusionConfiguration, + currentInclusionConfiguration InclusionConfiguration, + currentImplicitlyEnabled sets.Set[configv1.ClusterVersionCapability], +) sets.Set[configv1.ClusterVersionCapability] { // update manifest include check - manifestInclusionConfiguration.MajorVersion, + updateInclusionConfiguration.MajorVersion, // current manifest include check - manifestInclusionConfiguration.MajorVersion, + currentInclusionConfiguration.MajorVersion,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/manifest/manifest.go` around lines 60 - 87, The two IncludeAllowUnknownCapabilities calls must evaluate with different major-version contexts: keep using manifestInclusionConfiguration.MajorVersion (target major) when calling updateManifest.IncludeAllowUnknownCapabilities (and assigning updateManErr), but when iterating currentPayloadManifests call currentManifest.IncludeAllowUnknownCapabilities with the current payload major instead of reusing the same MajorVersion; obtain the current payload major from the current payload metadata (or add a currentMajor variable derived from currentManifest/currentPayload) and pass that as the MajorVersion argument so updateManifest uses the target major and currentManifest uses the current-major context.pkg/cvo/sync_worker.go (1)
451-452:⚠️ Potential issue | 🟠 MajorUse target payload major version for implicit-capability evaluation.
At Line [452], this still passes the current payload major (
w.payload.ParsedVersion.Major). For cross-major updates, implicit-capability resolution should use the target payload major (payloadUpdate.ParsedVersion.Major), or manifests can be evaluated against the wrong major-version filter.Suggested fix
- implicitlyEnabledCaps = capability.SortedList(payload.GetImplicitlyEnabledCapabilities(payloadUpdate.Manifests, w.payload.Manifests, - work.Capabilities, work.EnabledFeatureGates, ptr.To(w.payload.ParsedVersion.Major))) + implicitlyEnabledCaps = capability.SortedList(payload.GetImplicitlyEnabledCapabilities(payloadUpdate.Manifests, w.payload.Manifests, + work.Capabilities, work.EnabledFeatureGates, ptr.To(payloadUpdate.ParsedVersion.Major)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/sync_worker.go` around lines 451 - 452, The call computing implicitlyEnabledCaps uses the current payload major (w.payload.ParsedVersion.Major) but should use the target payload major from payloadUpdate for cross-major updates: update the call to payload.GetImplicitlyEnabledCapabilities so its last argument is ptr.To(payloadUpdate.ParsedVersion.Major) (keeping capability.SortedList and assignment to implicitlyEnabledCaps unchanged) to ensure manifest filtering uses the target payload major instead of w.payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/manifest/manifest.go`:
- Around line 60-87: The two IncludeAllowUnknownCapabilities calls must evaluate
with different major-version contexts: keep using
manifestInclusionConfiguration.MajorVersion (target major) when calling
updateManifest.IncludeAllowUnknownCapabilities (and assigning updateManErr), but
when iterating currentPayloadManifests call
currentManifest.IncludeAllowUnknownCapabilities with the current payload major
instead of reusing the same MajorVersion; obtain the current payload major from
the current payload metadata (or add a currentMajor variable derived from
currentManifest/currentPayload) and pass that as the MajorVersion argument so
updateManifest uses the target major and currentManifest uses the current-major
context.
In `@pkg/cvo/sync_worker.go`:
- Around line 451-452: The call computing implicitlyEnabledCaps uses the current
payload major (w.payload.ParsedVersion.Major) but should use the target payload
major from payloadUpdate for cross-major updates: update the call to
payload.GetImplicitlyEnabledCapabilities so its last argument is
ptr.To(payloadUpdate.ParsedVersion.Major) (keeping capability.SortedList and
assignment to implicitlyEnabledCaps unchanged) to ensure manifest filtering uses
the target payload major instead of w.payload.
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml`:
- Around line 4-11: The CRD's metadata.name is invalidly set to "noversion";
update metadata.name to match the required <plural>.<group> pattern by setting
it to "noversions.test.io" so it matches spec.names.plural ("noversions") and
spec.group ("test.io")—look for the metadata.name field and the
spec.names.plural / spec.group entries in this manifest (the resource currently
named "noversion") and change metadata.name to "noversions.test.io".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 366b0564-c600-4e8f-9a02-0bbc9a478b95
⛔ Files ignored due to path filters (21)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/manifest/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modlib/manifest/manifest.gopkg/cvo/cvo_scenarios_test.gopkg/cvo/featuregate_integration_test.gopkg/cvo/sync_worker.gopkg/cvo/testdata/majorversiontest-v5/manifests/.gitkeeppkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_40_version-4-or-5.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/image-referencespkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadatapkg/payload/payload.gopkg/payload/payload_test.gopkg/payload/render.gopkg/payload/render_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/image-references
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadata
- go.mod
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yaml
- pkg/payload/payload_test.go
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yaml
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_40_version-4-or-5.yaml
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml (1)
4-11:⚠️ Potential issue | 🟠 MajorUse a valid CRD name format (
<plural>.<group>).Line 4 uses
metadata.name: noversion, but withspec.names.plural: noversionsandspec.group: test.io, it should benoversions.test.io. This can hide real validation failures in tests that model CRD application.Suggested fix
metadata: - name: noversion + name: noversions.test.io annotations: include.release.openshift.io/self-managed-high-availability: "true"Run this read-only check to find CRD name mismatches in the same testdata set:
#!/bin/bash set -euo pipefail python - <<'PY' import glob import re import sys def parse_fields(path): name = plural = group = None section = None for raw in open(path, encoding="utf-8"): line = raw.rstrip("\n") if re.match(r'^\s*metadata:\s*$', line): section = "metadata" continue if re.match(r'^\s*spec:\s*$', line): section = "spec" continue if re.match(r'^\s*names:\s*$', line): section = "names" continue m = re.match(r'^\s*name:\s*("?[^"]+"?|\S+)\s*$', line) if m and section == "metadata" and name is None: name = m.group(1).strip('"') continue m = re.match(r'^\s*group:\s*("?[^"]+"?|\S+)\s*$', line) if m and section == "spec": group = m.group(1).strip('"') continue m = re.match(r'^\s*plural:\s*("?[^"]+"?|\S+)\s*$', line) if m and section == "names": plural = m.group(1).strip('"') continue return name, plural, group failed = False for path in sorted(glob.glob("pkg/cvo/testdata/majorversiontest-v5/release-manifests/*.yaml")): text = open(path, encoding="utf-8").read() if "kind: CustomResourceDefinition" not in text: continue name, plural, group = parse_fields(path) if not (name and plural and group): continue expected = f"{plural}.{group}" if name != expected: failed = True print(f"{path}: metadata.name={name!r}, expected={expected!r}") if failed: sys.exit(1) print("All CRD metadata.name values match <plural>.<group>.") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml` around lines 4 - 11, The CRD's metadata.name is incorrect — it should be the fully-qualified "<plural>.<group>" value; update metadata.name from "noversion" to "noversions.test.io" so that metadata.name equals spec.names.plural + "." + spec.group (ensure fields referenced: metadata.name, spec.names.plural, spec.group, kind: NoVersion).
🧹 Nitpick comments (1)
pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yaml (1)
3-13: CRD naming convention:metadata.nameshould be{plural}.{group}Kubernetes CRDs require
metadata.nameto follow the pattern{spec.names.plural}.{spec.group}. For this CRD, it should beversion4onlies.test.iorather thanversion4only.Since this is test data for manifest filtering logic (not applied to a real cluster), this may be intentional for simplicity. However, aligning with the real CRD naming convention would make the test data more representative.
Suggested fix
apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: - name: version4only + name: version4onlies.test.io annotations: include.release.openshift.io/self-managed-high-availability: "true" release.openshift.io/major-version: "4"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yaml` around lines 3 - 13, The CRD's metadata.name is incorrect: Kubernetes requires metadata.name to be "{spec.names.plural}.{spec.group}"; update the metadata.name value from "version4only" to "version4onlies.test.io" so it matches spec.names.plural ("version4onlies") and spec.group ("test.io") for the Version4Only CRD entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml`:
- Around line 4-11: The CRD's metadata.name is incorrect — it should be the
fully-qualified "<plural>.<group>" value; update metadata.name from "noversion"
to "noversions.test.io" so that metadata.name equals spec.names.plural + "." +
spec.group (ensure fields referenced: metadata.name, spec.names.plural,
spec.group, kind: NoVersion).
---
Nitpick comments:
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yaml`:
- Around line 3-13: The CRD's metadata.name is incorrect: Kubernetes requires
metadata.name to be "{spec.names.plural}.{spec.group}"; update the metadata.name
value from "version4only" to "version4onlies.test.io" so it matches
spec.names.plural ("version4onlies") and spec.group ("test.io") for the
Version4Only CRD entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a30e62f6-db56-4087-b9c8-63ce0a309981
⛔ Files ignored due to path filters (21)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/manifest/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modlib/manifest/manifest.gopkg/cvo/cvo_scenarios_test.gopkg/cvo/featuregate_integration_test.gopkg/cvo/sync_worker.gopkg/cvo/testdata/majorversiontest-v5/manifests/.gitkeeppkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_40_version-4-or-5.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/image-referencespkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadatapkg/payload/payload.gopkg/payload/payload_test.gopkg/payload/render.gopkg/payload/render_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/cvo/featuregate_integration_test.go
- go.mod
- pkg/cvo/sync_worker.go
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yaml
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadata
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yaml
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
47b4565 to
594b873
Compare
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
go.mod (1)
37-37:⚠️ Potential issue | 🟠 MajorAvoid merging with a personal fork replacement in
go.mod.At Line 37,
github.com/openshift/library-gois replaced with a personal fork. This should be removed (or limited to local/dev-only workflows) before merge to preserve provenance and reproducibility.Suggested change
-replace github.com/openshift/library-go => github.com/JoelSpeed/library-go v0.0.0-20260130121715-6e5ccfd0da42#!/bin/bash set -euo pipefail # Verify whether personal/library fork replacements remain in module file. rg -n '^replace\s+github.com/openshift/library-go' go.mod # Show required library-go entry for upstream pin visibility. rg -n 'github.com/openshift/library-go\s+v' go.mod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 37, Remove the personal-fork replace directive for github.com/openshift/library-go in go.mod (the line "replace github.com/openshift/library-go => github.com/JoelSpeed/library-go ...") before merging; either delete that replace, scope it to local/dev-only via a separate go.work or developer-only patch, or replace it with the appropriate upstream module version pin so provenance is preserved and builds are reproducible.pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml (1)
1-19:⚠️ Potential issue | 🟡 MinorUse valid CRD name format (
<plural>.<group>).The CRD
metadata.namemust follow the Kubernetes naming convention:<spec.names.plural>.<spec.group>. Currently set tonoversion, but should benoversions.test.ioto matchspec.names.plural: noversionsandspec.group: test.io.Suggested fix
metadata: - name: noversion + name: noversions.test.io annotations:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml` around lines 1 - 19, metadata.name is not following the `<plural>.<group>` convention: update the CRD's metadata.name to match spec.names.plural and spec.group by setting it to "noversions.test.io" (ensure you modify the metadata.name field in the CRD entry where metadata.name is currently "noversion" so it equals spec.names.plural + "." + spec.group).pkg/cvo/sync_worker.go (1)
451-452:⚠️ Potential issue | 🟠 MajorUse target payload major version for implicit capability evaluation.
At line 452,
w.payload.ParsedVersion.Major(current payload) is passed, butpayloadUpdate.ParsedVersion.Major(target payload) should be used. During cross-major upgrades (e.g., 4→5), evaluating update manifests against the current version could compute incorrect implicitly-enabled capabilities.Suggested fix
if w.payload != nil { implicitlyEnabledCaps = capability.SortedList(payload.GetImplicitlyEnabledCapabilities(payloadUpdate.Manifests, w.payload.Manifests, - work.Capabilities, work.EnabledFeatureGates, ptr.To(w.payload.ParsedVersion.Major))) + work.Capabilities, work.EnabledFeatureGates, ptr.To(payloadUpdate.ParsedVersion.Major))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/sync_worker.go` around lines 451 - 452, The call computing implicitlyEnabledCaps uses the current payload major version (w.payload.ParsedVersion.Major) but should use the target update's major version; update the argument passed to payload.GetImplicitlyEnabledCapabilities (inside capability.SortedList(...) that assigns implicitlyEnabledCaps) to use payloadUpdate.ParsedVersion.Major instead of w.payload.ParsedVersion.Major so implicit capability evaluation is performed against the target payload version.
🧹 Nitpick comments (1)
pkg/payload/render_test.go (1)
277-306: Consider removingnamespacefrom CRD test manifests.CRDs are cluster-scoped resources and don't have a namespace field in their metadata. While this may not break the test since it's just checking file inclusion/exclusion, it creates technically invalid CRD manifests.
💡 Suggested fix
func createTestManifestYAML(apiVersion, kind, name string, annotations map[string]string) string { yaml := `apiVersion: ` + apiVersion + ` kind: ` + kind + ` metadata: - name: ` + name + ` - namespace: test-namespace` + name: ` + name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/payload/render_test.go` around lines 277 - 306, The createTestManifestYAML helper currently always injects a namespace into metadata which produces invalid CRD manifests; update createTestManifestYAML so it omits the "namespace: test-namespace" line when the manifest represents a cluster-scoped CRD (e.g. when kind == "CustomResourceDefinition" and/or apiVersion indicates apiextensions.k8s.io/v1). Keep the metadata block and annotations for all kinds, but conditionally build the metadata string without the namespace for CRDs so the output is valid cluster-scoped YAML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Line 37: Remove the personal-fork replace directive for
github.com/openshift/library-go in go.mod (the line "replace
github.com/openshift/library-go => github.com/JoelSpeed/library-go ...") before
merging; either delete that replace, scope it to local/dev-only via a separate
go.work or developer-only patch, or replace it with the appropriate upstream
module version pin so provenance is preserved and builds are reproducible.
In `@pkg/cvo/sync_worker.go`:
- Around line 451-452: The call computing implicitlyEnabledCaps uses the current
payload major version (w.payload.ParsedVersion.Major) but should use the target
update's major version; update the argument passed to
payload.GetImplicitlyEnabledCapabilities (inside capability.SortedList(...) that
assigns implicitlyEnabledCaps) to use payloadUpdate.ParsedVersion.Major instead
of w.payload.ParsedVersion.Major so implicit capability evaluation is performed
against the target payload version.
In
`@pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yaml`:
- Around line 1-19: metadata.name is not following the `<plural>.<group>`
convention: update the CRD's metadata.name to match spec.names.plural and
spec.group by setting it to "noversions.test.io" (ensure you modify the
metadata.name field in the CRD entry where metadata.name is currently
"noversion" so it equals spec.names.plural + "." + spec.group).
---
Nitpick comments:
In `@pkg/payload/render_test.go`:
- Around line 277-306: The createTestManifestYAML helper currently always
injects a namespace into metadata which produces invalid CRD manifests; update
createTestManifestYAML so it omits the "namespace: test-namespace" line when the
manifest represents a cluster-scoped CRD (e.g. when kind ==
"CustomResourceDefinition" and/or apiVersion indicates apiextensions.k8s.io/v1).
Keep the metadata block and annotations for all kinds, but conditionally build
the metadata string without the namespace for CRDs so the output is valid
cluster-scoped YAML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2be614e1-9314-4a05-a3bf-973f15cfc63b
⛔ Files ignored due to path filters (21)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/manifest/manifest.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modlib/manifest/manifest.gopkg/cvo/cvo_scenarios_test.gopkg/cvo/featuregate_integration_test.gopkg/cvo/sync_worker.gopkg/cvo/testdata/majorversiontest-v5/manifests/.gitkeeppkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_10_no-version.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_20_version-4-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_40_version-4-or-5.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yamlpkg/cvo/testdata/majorversiontest-v5/release-manifests/image-referencespkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadatapkg/payload/payload.gopkg/payload/payload_test.gopkg/payload/render.gopkg/payload/render_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/payload/payload.go
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/release-metadata
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_50_exclude-version-3.yaml
- pkg/cvo/testdata/majorversiontest-v5/release-manifests/0000_30_version-5-only.yaml
JoelSpeed
force-pushed
the
major-version-inclusion
branch
from
a3ef6d2 to
4895c6d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta, JoelSpeed, wking 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 |
|
Nothing to test here at the moment, we're just setting up for a future world where there are different v4 and v5 feature gates in the main branch. We can test then, and iterate if we find gaps in this implementation. /verified by @wking |
openshift-ci-robot
added
the
verified
|
@wking: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hrm, another slow ack on upgrade-out-of-change. I'm not sure how this pull could trigger that, but the test seems suspiciously dead: 
And hypershift-conformance has: which I suspect means something about the new inclusion/exclusion rules needs to be ported to HyperShift's weird manifest pre-processing logic. Dropping into gathered management-cluster logs: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1282/pull-ci-openshift-cluster-version-operator-main-e2e-hypershift-conformance/2032547489449512960/artifacts/e2e-hypershift-conformance/dump/artifacts/namespaces/clusters-fb412d747c637a182064/core/pods/logs/cluster-version-operator-568ccf9cc9-r7jj9-cluster-version-operator.log | grep -i clusterversionoperator
I0313 21:45:41.505858 1 start.go:22] ClusterVersionOperator v1.0.0-1667-g6c8aa9b5-dirty
I0313 21:45:41.727487 1 payload.go:217] excluding Filename: "0000_00_cluster-version-operator_01_clusterversionoperators.crd.yaml" Group: "apiextensions.k8s.io" Kind: "CustomResourceDefinition" Name: "clusterversionoperators.operator.openshift.io": "Default" is required, and release.openshift.io/feature-set=CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgrade
I0313 21:45:42.629962 1 cvo.go:468] Starting ClusterVersionOperator with minimum reconcile period 3m2.389972914s
I0313 21:45:42.630028 1 cvo.go:518] The ClusterVersionOperatorConfiguration feature gate is disabled or HyperShift is detected; the configuration sync routine will not run.
I0313 21:45:42.633986 1 payload.go:217] excluding Filename: "0000_00_cluster-version-operator_01_clusterversionoperators.crd.yaml" Group: "apiextensions.k8s.io" Kind: "CustomResourceDefinition" Name: "clusterversionoperators.operator.openshift.io": "Default" is required, and release.openshift.io/feature-set=CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgradeSo that looks appropriate. What added the CRD to the cluster? Over to hosted-cluster CRDs: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1282/pull-ci-openshift-cluster-version-operator-main-e2e-hypershift-conformance/2032547489449512960/artifacts/e2e-hypershift-conformance/dump/artifacts/hostedcluster-fb412d747c637a182064/cluster-scoped-resources/apiextensions.k8s.io/customresourcedefinitions/clusterversionoperators.operator.openshift.io.yaml | yaml2json | jq -r '.metadata.managedFields[] | .time + " " + .operation + " " + .subresource + " " + .manager'
2026-03-13T21:45:38Z Update status kube-apiserver
2026-03-13T21:45:38Z Update kubectl-client-side-apply
$ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1282/pull-ci-openshift-cluster-version-operator-main-e2e-hypershift-conformance/2032547489449512960/artifacts/e2e-hypershift-conformance/dump/artifacts/namespaces/clusters-fb412d747c637a182064/core/pods/logs/cluster-version-operator-568ccf9cc9-r7jj9-bootstrap.log | cat
namespace/openshift-config created
...
Applying CVO bootstrap manifests...
...
customresourcedefinition.apiextensions.k8s.io/clusterversionoperators.operator.openshift.io created
customresourcedefinition.apiextensions.k8s.io/clusterversions.config.openshift.io unchanged
...
Bootstrap manifests applied successfully.So something around here needs touching. |
|
/hold for that HyperShift work #1282 (comment) |
openshift-ci
bot
added
the
do-not-merge/hold
|
@JoelSpeed: The following tests 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. |
|
Thanks, Trevor. Checking the PR's changes, specifically any CRD filename changes due to the HyperShift processing. Bingo: 
So the HyperShift depends on the filenames to follow a defined pattern; the filename changes in the o/api break that logic. Not sure if this explains all of Trevor's comments at the moment, but it would explain some. |
|
A small note. This is how the vendored CRDs in the CVO repository get into the CVO container image. |
|
Amusingly this comment described their ideal state, which is something that is entirely possible
|
|
/testwith openshift/cluster-version-operator/main/e2e-hypershift-conformance openshift/hypershift#7984 |
|
Thanks, Joel, for coming up with a PR in the HyperShift repository! The blocked openshift/api bump is starting to block more and more efforts (including me starting today). I have created https://redhat.atlassian.net/browse/OCPBUGS-78705 to represent the issue in the meantime. |
….openshift.io/hypershift-bootstrap HyperShift currently massages release-image manifests [1] and then applies the ones it expects the cluster-version operator to need [2] in order to get that Deployment running. That can cause problems like installing the v1alpha1 clusterversionoperators.operator.openshift.io [3] when HyperShift is using filename-based logic and assuming the remaining manifests are all necessary to bootstrap the CVO [2]: oc apply -f /var/payload/manifests With this commit, I'm adding the include.release.openshift.io/hypershift-bootstrap annotation to the manifests we actually need for HyperShift bootstrapping, to allow the HyperShift operator to move to something like: oc create -f $(grep -rl include.release.openshift.io/hypershift-bootstrap /var/payload/manifests) For the two manifests we maintain locally, I've added the grep target as an annotation. We need the ClusterVersion CustomResourceDefinition in place too, see 7480a5f (Get cluster version object earlier in startup, 2022-03-11, openshift#741). But we are unlikely to need any fancy feature-gated knobs in the ClusterVersion CRD just to get the CVO going. So with this commit I'm appending a YAML comment with the grep target to the Default CRD, and the expectation is that: 1. The bootstrapping creates the Default ClusterVersion CRD. 2. The control-plane operator populates it with enough to get the initial CVO running (e.g. spec.capabilities to exclude some capabilities, because once installed, capabilities cannot be uninstalled [4]) 2. HyperShift applies the default portions of the spec when creating a ClusterVersion resource. 3. CVO comes up, reads in the initial CluserVersion and FeatureGate and whatever else it needs, and if necessary, updates the ClusterVersion CRD. 4. HyperShift populates any feature-gated ClusterVersion knobs, now that those slots exist. 5. Post-install cluster lifecycle continues to have the CVO managing its own resources in the cluster, no more need for HyperShift bootstrapping. It would be elegant to have the new grep target as an annotation in the CVO, but for a flag of such limited utility, finding a way to reliably inject it from the Dockerfile seems like more trouble than it would be worth, which is why I'm using the YAML comment approach. [1]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go#L205-L241 [2]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-version-operator/deployment.yaml#L89-L104 [3]: openshift#1282 (comment) [4]: https://github.com/openshift/enhancements/blob/2b38513b8661632f08e64f4acc3b856e842f8669/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled
|
/testwith openshift/cluster-version-operator/main/e2e-hypershift-conformance openshift/hypershift#7984 I had an error in the previous push, should be fixed now |
….openshift.io/hypershift-bootstrap HyperShift currently massages release-image manifests [1] and then applies the ones it expects the cluster-version operator to need [2] in order to get that Deployment running. That can cause problems like installing the v1alpha1 clusterversionoperators.operator.openshift.io [3] when HyperShift is using filename-based logic and assuming the remaining manifests are all necessary to bootstrap the CVO [2]: oc apply -f /var/payload/manifests With this commit, I'm adding the include.release.openshift.io/hypershift-bootstrap annotation to the manifests we actually need for HyperShift bootstrapping, to allow the HyperShift operator to move to something like: oc create -f $(grep -rl include.release.openshift.io/hypershift-bootstrap /var/payload/manifests) For the two manifests we maintain locally, I've added the grep target as an annotation. We need the ClusterVersion CustomResourceDefinition in place too, see 7480a5f (Get cluster version object earlier in startup, 2022-03-11, openshift#741). But we are unlikely to need any fancy feature-gated knobs in the ClusterVersion CRD just to get the CVO going. So with this commit I'm appending a YAML comment with the grep target to the Default CRD, and the expectation is that: 1. The bootstrapping creates the Default ClusterVersion CRD. 2. The HyperShift control-plane operator populates it with enough to get the initial CVO running (e.g. spec.capabilities to exclude some capabilities, because once installed, capabilities cannot be uninstalled [4]). 3. CVO comes up, reads in the initial CluserVersion and FeatureGate and whatever else it needs, and if necessary, updates the ClusterVersion CRD. 4. HyperShift populates any feature-gated ClusterVersion knobs, now that those slots exist. 5. Post-install cluster lifecycle continues to have the CVO managing its own resources in the cluster, no more need for HyperShift bootstrapping. It would be elegant to have the new grep target as an annotation in the CVO, but for a flag of such limited utility, finding a way to reliably inject it from the Dockerfile seems like more trouble than it would be worth, which is why I'm using the YAML comment approach. [1]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go#L205-L241 [2]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-version-operator/deployment.yaml#L89-L104 [3]: openshift#1282 (comment) [4]: https://github.com/openshift/enhancements/blob/2b38513b8661632f08e64f4acc3b856e842f8669/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled
….openshift.io/bootstrap-cluster-version-operator HyperShift currently massages release-image manifests [1] and then applies the ones it expects the cluster-version operator to need [2] in order to get that Deployment running. That can cause problems like installing the v1alpha1 clusterversionoperators.operator.openshift.io [3] when HyperShift is using filename-based logic and assuming the remaining manifests are all necessary to bootstrap the CVO [2]: oc apply -f /var/payload/manifests With this commit, I'm adding the include.release.openshift.io/bootstrap-cluster-version-operator annotation to the manifests we actually need for HyperShift bootstrapping, to allow the HyperShift operator to move to something like: oc create -f $(grep -rl 'include.release.openshift.io/bootstrap-cluster-version-operator: .*hypershift' /var/payload/manifests) For the two manifests we maintain locally, I've added the grep target as an annotation. We need the ClusterVersion CustomResourceDefinition in place too, see 7480a5f (Get cluster version object earlier in startup, 2022-03-11, openshift#741). But we are unlikely to need any fancy feature-gated knobs in the ClusterVersion CRD just to get the CVO going. So with this commit I'm appending a YAML comment with the grep target to the Default CRD, and the expectation is that: 1. The bootstrapping creates the Default ClusterVersion CRD. 2. The HyperShift control-plane operator populates it with enough to get the initial CVO running (e.g. spec.capabilities to exclude some capabilities, because once installed, capabilities cannot be uninstalled [4]). 3. CVO comes up, reads in the initial CluserVersion and FeatureGate and whatever else it needs, and if necessary, updates the ClusterVersion CRD. 4. HyperShift populates any feature-gated ClusterVersion knobs, now that those slots exist. 5. Post-install cluster lifecycle continues to have the CVO managing its own resources in the cluster, no more need for HyperShift bootstrapping. It would be elegant to have the new grep target as an annotation in the CVO, but for a flag of such limited utility, finding a way to reliably inject it from the Dockerfile seems like more trouble than it would be worth, which is why I'm using the YAML comment approach. [1]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go#L205-L241 [2]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-version-operator/deployment.yaml#L89-L104 [3]: openshift#1282 (comment) [4]: https://github.com/openshift/enhancements/blob/2b38513b8661632f08e64f4acc3b856e842f8669/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled
….openshift.io/bootstrap-cluster-version-operator HyperShift currently massages release-image manifests [1] and then applies the ones it expects the cluster-version operator to need [2] in order to get that Deployment running. That can cause problems like installing the v1alpha1 clusterversionoperators.operator.openshift.io [3] when HyperShift is using filename-based logic and assuming the remaining manifests are all necessary to bootstrap the CVO [2]: oc apply -f /var/payload/manifests With this commit, I'm adding the include.release.openshift.io/bootstrap-cluster-version-operator annotation to the manifests we actually need for HyperShift bootstrapping, to allow the HyperShift operator to move to something like: oc create -f $(grep -rl 'include.release.openshift.io/bootstrap-cluster-version-operator: .*hypershift' /var/payload/manifests) For the two manifests we maintain locally, I've added the grep target as an annotation. We need the ClusterVersion CustomResourceDefinition in place too, see 7480a5f (Get cluster version object earlier in startup, 2022-03-11, openshift#741). But we are unlikely to need any fancy feature-gated knobs in the ClusterVersion CRD just to get the CVO going. So with this commit I'm appending a YAML comment with the grep target to the Default CRD, and the expectation is that: 1. The bootstrapping creates the Default ClusterVersion CRD. 2. The HyperShift control-plane operator populates it with enough to get the initial CVO running (e.g. spec.capabilities to exclude some capabilities, because once installed, capabilities cannot be uninstalled [4]). 3. CVO comes up, reads in the initial CluserVersion and FeatureGate and whatever else it needs, and if necessary, updates the ClusterVersion CRD. 4. HyperShift populates any feature-gated ClusterVersion knobs, now that those slots exist. 5. Post-install cluster lifecycle continues to have the CVO managing its own resources in the cluster, no more need for HyperShift bootstrapping. It would be elegant to have the new grep target as an annotation in the CVO, but for a flag of such limited utility, finding a way to reliably inject it from the Dockerfile seems like more trouble than it would be worth, which is why I'm using the YAML comment approach. [1]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go#L205-L241 [2]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-version-operator/deployment.yaml#L89-L104 [3]: openshift#1282 (comment) [4]: https://github.com/openshift/enhancements/blob/2b38513b8661632f08e64f4acc3b856e842f8669/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled
….openshift.io/bootstrap-cluster-version-operator HyperShift currently massages release-image manifests [1] and then applies the ones it expects the cluster-version operator to need [2] in order to get that Deployment running. That can cause problems like installing the v1alpha1 clusterversionoperators.operator.openshift.io [3] when HyperShift is using filename-based logic and assuming the remaining manifests are all necessary to bootstrap the CVO [2]: oc apply -f /var/payload/manifests With this commit, I'm adding the include.release.openshift.io/bootstrap-cluster-version-operator annotation to the manifests we actually need for HyperShift bootstrapping, to allow the HyperShift operator to move to something like: oc create -f $(grep -rl 'include.release.openshift.io/bootstrap-cluster-version-operator: .*hypershift' /var/payload/manifests) For the two manifests we maintain locally, I've added the grep target as an annotation. We need the ClusterVersion CustomResourceDefinition in place too, see 7480a5f (Get cluster version object earlier in startup, 2022-03-11, cache.WaitForCacheSync in CVO.Run, or we stick on: I0318 21:30:46.499731 1 reflector.go:404] "Listing and watching" type="*v1.ClusterOperator" reflector="github.com/openshift/client-go/config/informers/externalversions/factory.go:125" E0318 21:30:46.500825 1 reflector.go:205] "Failed to watch" err="failed to list *v1.ClusterOperator: the server could not find the requested resource (get clusteroperators.config.openshift.io)" logger="UnhandledError" reflector="github.com/openshift/client-go/config/informers/externalversions/factory.go:125" type="*v1.ClusterOperator" But we are unlikely to need any fancy feature-gated knobs in the CRDs just to get the CVO going. So with this commit I'm appending a YAML comment with the grep target to the Default CRD, and the expectation is that: 1. The bootstrapping creates the Default ClusterVersion CRD. 2. The HyperShift control-plane operator populates it with enough to get the initial CVO running (e.g. spec.capabilities to exclude some capabilities, because once installed, capabilities cannot be uninstalled [4]). 3. CVO comes up, reads in the initial CluserVersion and FeatureGate and whatever else it needs, and if necessary, updates the ClusterVersion CRD. 4. HyperShift populates any feature-gated ClusterVersion knobs, now that those slots exist. 5. Post-install cluster lifecycle continues to have the CVO managing its own resources in the cluster, no more need for HyperShift bootstrapping. It would be elegant to have the new grep target as an annotation in the CVO, but for a flag of such limited utility, finding a way to reliably inject it from the Dockerfile seems like more trouble than it would be worth, which is why I'm using the YAML comment approach. [1]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go#L205-L241 [2]: https://github.com/openshift/hypershift/blob/a85cd0a7ed067748f74a7869955ebd05a01912b8/control-plane-operator/controllers/hostedcontrolplane/v2/assets/cluster-version-operator/deployment.yaml#L89-L104 [3]: openshift#1282 (comment) [4]: https://github.com/openshift/enhancements/blob/2b38513b8661632f08e64f4acc3b856e842f8669/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled
|
openshift/hypershift#7984 merged: /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
4 participants
This introduces a new CVO manifest filter based on the OCP major version.
The intended usage would allow folks to say "this manifest should be deployed on OCP 5 and skipped on OCP 4"
The library-go component here is limiting this feature to FeatureGate and CustomResourceDefinition only for now.
This is not intended for use beyond the feature gating mechanisms we have today. We expect FeatureGate and CustomResourceDefinition yamls to vary by release, but all other inclusion/exclusion should be handled by existing mechanisms like the feature-gate inclusion mechanism.
Example usage:
Summary by CodeRabbit
New Features
Tests
Chores