Require APIExcludedFields Versions to be set#2727
Require APIExcludedFields Versions to be set#2727JoelSpeed wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThe change makes the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/XS
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apiextensions/v1alpha1/types_compatibilityrequirement.go (1)
157-170:⚠️ Potential issue | 🟡 MinorUpdate
versionsfield documentation to match required validation.The comment still says “When not specified...” but Line 169 makes
versionsmandatory. Please align docs to avoid contradictory API guidance.✏️ Suggested doc fix
// versions are the API versions the field is excluded from. - // When not specified, the field is excluded from all versions. + // This field is required and must explicitly list the API versions + // the field is excluded from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apiextensions/v1alpha1/types_compatibilityrequirement.go` around lines 157 - 170, The comment for the Versions field contradicts its validation: update the documentation for the Versions []APIVersionString field (symbol: Versions, type: APIVersionString) to remove the “When not specified, the field is excluded from all versions.” language and instead state that Versions is required and must contain 1–32 items (each ≤63 chars, lowercase alphanumeric and hyphens, starts with a letter, ends with alphanumeric), reflecting the +kubebuilder:validation:MinItems=1, MaxItems=32 and +required tags so the doc and validation match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apiextensions/v1alpha1/types_compatibilityrequirement.go`:
- Around line 169-170: The change made Versions (the field Versions
[]APIVersionString on the CompatibilityRequirement struct) required by adding //
+required, which breaks backward compatibility for legacy objects that omit
versions; revert the // +required tag on Versions (or alternatively implement a
defaulting step in the API defaulting webhook that supplies a sensible default
like "all versions") so existing CRs with excludedFields that only specify path
remain valid; update the CompatibilityRequirement field annotation for Versions
accordingly and, if you choose defaulting, implement the logic in the defaulting
routine tied to CompatibilityRequirement to populate Versions when nil.
---
Outside diff comments:
In `@apiextensions/v1alpha1/types_compatibilityrequirement.go`:
- Around line 157-170: The comment for the Versions field contradicts its
validation: update the documentation for the Versions []APIVersionString field
(symbol: Versions, type: APIVersionString) to remove the “When not specified,
the field is excluded from all versions.” language and instead state that
Versions is required and must contain 1–32 items (each ≤63 chars, lowercase
alphanumeric and hyphens, starts with a letter, ends with alphanumeric),
reflecting the +kubebuilder:validation:MinItems=1, MaxItems=32 and +required
tags so the doc and validation match.
ℹ️ Review info
Configuration used: Path: .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 (3)
apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yamlis excluded by!**/zz_generated.crd-manifests/*apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (2)
apiextensions/v1alpha1/types_compatibilityrequirement.gopayload-manifests/crds/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yaml
JoelSpeed
force-pushed
the
excluded-versions-required
branch
from
7e1fe9a to
3cfb57e
Compare
openshift-ci
bot
added
size/S
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apiextensions/v1alpha1/tests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yaml (1)
168-174: Consider adding a negative test for omittingversionson an excludedFields entry.The PR makes
versionsrequired, but there is no test case that verifies the server rejects an excludedFields entry withoutversions. A test like the following would directly validate the new constraint:- name: Should not be able to create CompatibilityRequirement with excludedFields missing versions initial: | apiVersion: apiextensions.openshift.io/v1alpha1 kind: CompatibilityRequirement metadata: name: test-requirement spec: compatibilitySchema: customResourceDefinition: type: YAML data: | ... requiredVersions: defaultSelection: StorageOnly excludedFields: - path: spec.fieldToExclude expectedError: "spec.compatibilitySchema.excludedFields[0].versions: Required value"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apiextensions/v1alpha1/tests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yaml` around lines 168 - 174, Add a negative test that attempts to create a CompatibilityRequirement resource with spec.compatibilitySchema.excludedFields entries missing the required versions field and assert the server rejects it; specifically add a test case named like "Should not be able to create CompatibilityRequirement with excludedFields missing versions" that posts a CompatibilityRequirement (kind: CompatibilityRequirement, apiVersion: apiextensions.openshift.io/v1alpha1) whose spec.compatibilitySchema.customResourceDefinition/data contains excludedFields with an entry that has only path: spec.fieldToExclude and no versions, and set expectedError to contain "spec.compatibilitySchema.excludedFields[0].versions: Required value" to validate the new versions-required constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apiextensions/v1alpha1/tests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yaml`:
- Around line 168-174: Add a negative test that attempts to create a
CompatibilityRequirement resource with spec.compatibilitySchema.excludedFields
entries missing the required versions field and assert the server rejects it;
specifically add a test case named like "Should not be able to create
CompatibilityRequirement with excludedFields missing versions" that posts a
CompatibilityRequirement (kind: CompatibilityRequirement, apiVersion:
apiextensions.openshift.io/v1alpha1) whose
spec.compatibilitySchema.customResourceDefinition/data contains excludedFields
with an entry that has only path: spec.fieldToExclude and no versions, and set
expectedError to contain "spec.compatibilitySchema.excludedFields[0].versions:
Required value" to validate the new versions-required constraint.
ℹ️ Review info
Configuration used: Path: .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 (3)
apiextensions/v1alpha1/zz_generated.crd-manifests/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yamlis excluded by!**/zz_generated.crd-manifests/*apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (3)
apiextensions/v1alpha1/tests/compatibilityrequirements.apiextensions.openshift.io/CRDCompatibilityRequirementOperator.yamlapiextensions/v1alpha1/types_compatibilityrequirement.gopayload-manifests/crds/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- apiextensions/v1alpha1/types_compatibilityrequirement.go
- payload-manifests/crds/0000_20_crd-compatibility-checker_01_compatibilityrequirements.crd.yaml
|
/override ci/prow/verify-crd-schema As this is a tech preview feature, we are happy to make this technically breaking change |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify 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. |
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
/lgtm
We discussed this at length. It wasn't the original plan, but we think the producer can add this information without undue toil. The upside is better UX if you specify it incorrectly.
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
|
Scheduling tests matching the |
|
/approve |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
https://redhat-internal.slack.com/archives/GE2HQ9QP4/p1772028728668839 discussion overview |
|
@JoelSpeed: 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. |
|
/retest |
4 participants
Through discussion, we've realised that having this field optional creates ambiguity around the expected behaviour. We would rather this be completely declarative and require users to set which versions they want the exclusion to apply to.
Since this API is new in 4.22 and only shipped in tech preview, this is not actually a breaking change so should be fine to merge (looking at you crdify)