OCPCLOUD-3214: Add ClusterAPI config #2564
Conversation
|
Pipeline controller notification For optional jobs, comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request references OCPCLOUD-3214 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 story to target the "4.21.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. |
|
Hello @mdbooth! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
mdbooth
force-pushed
the
ccapio-staged
branch
from
1eb67fc to
5e7a826
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| // The maximum number of unmanagedCustomResourceDefinitions is 128. | ||
| // | ||
| // +optional | ||
| // +listType=set |
There was a problem hiding this comment.
Q: is this resource created by each actor or will there be one instance with co-ownership from different actors?
Example 1:
- 1 ClusterAPI object created by HyperShift
- 1 ClusterAPI object created by ACM
Example 2:
- 1 ClusterAPI object, some entries in .spec.unmanagedCustomResourceDefinitions are from HyperShift, others are from ACM.
There was a problem hiding this comment.
You can't have duplicates in listType=set, so multiple actors would own the same entry if that happened.
Each actor simply needs to submit an SSA transaction with their own requirements without reference to any other actors.
| } | ||
|
|
||
| // ClusterAPISpec defines the desired configuration of the capi-operator. | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.unmanagedCustomResourceDefinitions) || has(self.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set" |
There was a problem hiding this comment.
We validate removal to not happen, do we also validate deletion of the object to not happen?
There was a problem hiding this comment.
Interesting 🤔
I'm not convinced we can validate this here. I suspect we don't want to validate this at all, though: if the user is deleting ClusterOperators then they're already having a bad day. They've chosen this course of action and they're going to do it regardless. Whatever barriers we raise are just another in a succession of irritations that will be circumvented: disabling CVO, deleting VAPs, webhooks, etc. We can discourage the use of footguns, but ultimately we can't uninvent them.
| # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set" | ||
| # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place. | ||
| # The validation is still enforced at the API level via the XValidation rule. |
There was a problem hiding this comment.
Shouldn't this work? (I tried it)
| # TODO: Cannot test the XValidation rule "unmanagedCustomResourceDefinitions cannot be unset once set" | |
| # because MinProperties=1 on ClusterAPISpec prevents creating spec: {} in the first place. | |
| # The validation is still enforced at the API level via the XValidation rule. | |
| - name: Should not allow unsetting unmanagedCustomResourceDefinitions | |
| initial: | | |
| apiVersion: operator.openshift.io/v1alpha1 | |
| kind: ClusterAPI | |
| metadata: | |
| name: cluster | |
| spec: | |
| unmanagedCustomResourceDefinitions: | |
| - clusters.cluster.x-k8s.io | |
| - machines.cluster.x-k8s.io | |
| updated: | | |
| apiVersion: operator.openshift.io/v1alpha1 | |
| kind: ClusterAPI | |
| metadata: | |
| name: cluster | |
| spec: | |
| unmanagedCustomResourceDefinitions: null | |
| expectedError: "unmanagedCustomResourceDefinitions cannot be unset once set" |
There was a problem hiding this comment.
I don't think set to null is the same as unset.
However, I decided to go a different direction with this: I made spec optional. I think this should be the default for a ClusterOperator, tbh.
mdbooth
force-pushed
the
ccapio-staged
branch
2 times, most recently
from
c496532 to
465771a
Compare
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
General structure looks fine, some questions inline
Also, based on Claude suggestion, all optional fields (spec, status, UnmanagedCustomResourceDefinitions, TargetConfigMaps, ActiveConfigMaps) should have a godoc line explaining what happens if the value is empty. (example: When omitted, the capi-operator will use default configuration with all CRDs managed by the operator. in the spec, assuming we keep spec optional)
| // | ||
| // Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. | ||
| // +openshift:compatibility-gen:level=4 | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set" |
There was a problem hiding this comment.
I think for validations, it's best to have it explicitly in the godoc as well if possible (so in this case, maybe just unmanagedCustomResourceDefinitions cannot be unset once set as a godoc line is fine?).
Side note, I thought that if we don't edit unmanagedCustomResourceDefinitions but instead delete the whole spec field after setting it at some point, self.spec.unmanagedCustomResourceDefinitions would maybe not evaluate correctly, but it seems that CEL can handle that null exception (and you have a test case above covering it)
There was a problem hiding this comment.
Hmm, I'm not sure that godoc lives here, though. This isn't where a user is going to to go looking for it; they're going to be looking at the unmanagedCustomResourceDefinitions field. It's only here because of a CEL implementation detail.
There was a problem hiding this comment.
Good point. I'm wondering if we should move this validation to the ClusterAPISpec object then, since it doesn't refer to anything outside of spec
| // Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. | ||
| // +openshift:compatibility-gen:level=4 | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set" | ||
| type ClusterAPI struct { |
There was a problem hiding this comment.
So, like the other operator types, I assume this will be a singleton. Interestingly enough the only one to enforce it is the olm operator: https://github.com/openshift/api/blob/master/operator/v1alpha1/types_olm.go#L25
I think generally speaking it's a good pattern to have, so probably worth adding that? Unless I'm misunderstanding how this can be configured.
|
|
||
| // spec is the specification of the desired behavior of the capi-operator. | ||
| // +optional | ||
| Spec *ClusterAPISpec `json:"spec,omitempty"` |
There was a problem hiding this comment.
Based on updated guidance, I think now we only pointers if there's a difference between the unset and empty struct, otherwise having it same as the status field below (not a pointer, omitzero) is the best option.
In this case, since "" and empty should both mean "no CRDs are unmanaged", I think this falls under that umbrella?
The other way we could maybe do this (and I don't know what is "better" conventions-wise) is to instead make this required, keep unmanagedCustomResourceDefinitions optional, move the // +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || !has(oldSelf.spec.unmanagedCustomResourceDefinitions) || has(self.spec.unmanagedCustomResourceDefinitions)",message="unmanagedCustomResourceDefinitions cannot be unset once set" validation here to the spec, and remove the need for nil checking generally. This might be more future proof in case we want to add more spec fields in the future.
There was a problem hiding this comment.
I'll try setting this back to a zeroable struct, but I recall having previously flipped this so I have a suspicion there's a reason that doesn't work down the line somewhere.
There was a problem hiding this comment.
The linter complains:
operator/v1alpha1/types_clusterapi.go:35:2: optionalfields: field Spec has a valid zero value ({}), but the validation is not complete (e.g. min properties/adding required fields). The field should be a pointer to allow the zero value to be set. If the zero value is not a valid use case, complete the validation and remove the pointer. (kubeapilinter)
Spec ClusterAPISpec `json:"spec,omitempty"`
I feel like the same could be achieved by omitting omitempty from the json marker, but the linter doesn't permit that either. I suspect we're trying to cover multiple use cases with this marshalling behaviour, so perhaps that would break something else?
For now the linter requires this to be a pointer 🤷
There was a problem hiding this comment.
Will check this with the API team.
|
|
||
| // ClusterAPISpec defines the desired configuration of the capi-operator. | ||
| type ClusterAPISpec struct { | ||
| // unmanagedCustomResourceDefinitions is a list of ClusterResourceDefinition (CRD) |
There was a problem hiding this comment.
Presumably, this is only a set of CRDs that CAPI operator would use, so e.g. if I set "MachineConfigs" here, nothing would happen since CAPI doesn't manage MachineConfigs, nor can it tell CVO/MCO to not deploy/use some other api version.
Would there be a way for the user to discover what CRDs is able to be unmanaged here? Is that a big list or something we can add to the godoc/validator?
Reading this and openshift/enhancements#1845 I had initially thought that this is a generic list you can apply to any CRD but that shouldn't be the case right?
There was a problem hiding this comment.
I think this list is fundamentally un-validatable. The problem is we have a documented use case where the user is expected to add an entry for a CRD which:
- May not yet be present in the cluster
- Is not part of any manifests we currently know about
I'd be hesitant to add a heuristic which will inevitably turn out to be wrong, especially if it's also complex to implement. I'd prefer to leave this to the documentation.
There was a problem hiding this comment.
I think that makes sense. By documentation do you mean part of the godoc here? Or as part of OCP documentation tracked elsewhere?
And if I were to provide CRDs which the capi-operator doesn't understand, would it error or silently skip over?
| } | ||
|
|
||
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:MinProperties=1 |
There was a problem hiding this comment.
Question: I assume only CAPI-operator would be updating the status of this object. Does the validation here help prevent a specific scenario?
(should be fine to have, just curious why this validation was added)
There was a problem hiding this comment.
Pretty sure I added it to make the linter happy without having to make Status a pointer.
| // ClusterAPIStatus describes the current state of the capi-operator. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type ClusterAPIStatus struct { | ||
| // targetConfigMaps is a list of ConfigMap names that the staging controller |
There was a problem hiding this comment.
I'm probably missing some context but I'm not 100% sure what these two configmap status field entails. I assume these are here for an admin to see? Are these reflecting the status of some other action the admin took elsewhere? Or something that has to do with the CRDs they just defined in the spec? (Just from an ELI5 perspective the status fields are confusing, if there's some other way to add some more context I think that would be helpful)
There was a problem hiding this comment.
I need to re-write this (again), but this is for internal state of the installer controller.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis pull request introduces the ClusterAPI v1alpha1 custom resource type to the OpenShift operator package, including full Kubernetes API integration. The changes add type definitions with fields for managing revisions, components, and unmanaged custom resource definitions. Supporting infrastructure includes OpenAPI schema generation, deepcopy method implementations, scheme registration, Swagger documentation, and a feature-gated CRD manifest entry. Comprehensive test configuration exercises validation scenarios for resource creation, updates, immutability constraints, and status subresource behavior. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 60773-60792: The generated OpenAPI function
schema_openshift_api_operator_v1alpha1_ClusterAPIInstallerComponentImage is
missing the string validation constraints from
machineosconfigv1.ImageDigestFormat; update the "digest" property's SchemaProps
to include MinLength: 1, MaxLength: 447 and the appropriate Pattern (the regex
that enforces host[:port][/namespace]/name@sha256:<64-lowercase-hex> or at
minimum the @sha256:[a-f0-9]{64} portion used in the kubebuilder XValidation),
so the OpenAPI output matches the source kubebuilder markers and preserves the
same validation semantics.
In @operator/v1alpha1/zz_generated.swagger_doc_generated.go:
- Around line 167-177: Add a type-level doc comment to the
ClusterAPIInstallerRevision type declaration in
operator/v1alpha1/types_clusterapi.go so the swagger generator emits the
empty-string key; specifically, locate the type named
ClusterAPIInstallerRevision and add a brief comment line immediately above it
(e.g., "// ClusterAPIInstallerRevision ...") describing the type, then re-run
hack/update-swagger-docs.sh to regenerate zz_generated.swagger_doc_generated.go
which will include the "" key in map_ClusterAPIInstallerRevision.
🧹 Nitpick comments (2)
operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
17-17: Consider adding printer columns for better kubectl output.The PrinterColumns array is empty, which means
kubectl get clusterapiswill only show default columns (NAME, AGE). Adding custom columns for key status fields would improve usability.Example columns to consider:
- Status/Phase
- Observed generation
- Current revision
This can be addressed in a follow-up as the API stabilizes.
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (1)
460-551: LGTM!The revision ordering tests correctly enforce that new revisions must have a revision number greater than all existing revisions, aligning with the monotonically increasing requirement.
Consider adding a test case for attempting to delete all revisions (leaving an empty list), which should fail due to the
MinItems=1constraint onrevisions. This would provide complete coverage of the boundary conditions.
📜 Review details
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 (4)
operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1alpha1/zz_generated.featuregated-crd-manifests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (8)
openapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1alpha1/register.gooperator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yamloperator/v1alpha1/types_clusterapi.gooperator/v1alpha1/zz_generated.deepcopy.gooperator/v1alpha1/zz_generated.featuregated-crd-manifests.yamloperator/v1alpha1/zz_generated.swagger_doc_generated.go
🧰 Additional context used
🧬 Code graph analysis (3)
operator/v1alpha1/types_clusterapi.go (1)
operator/v1alpha1/zz_generated.swagger_doc_generated.go (6)
ClusterAPISpec(194-196)ClusterAPIStatus(205-207)ClusterAPIInstallerRevision(175-177)ClusterAPIInstallerComponent(154-156)ClusterAPIInstallerComponentImage(163-165)ClusterAPI(145-147)
operator/v1alpha1/zz_generated.swagger_doc_generated.go (1)
operator/v1alpha1/types_clusterapi.go (7)
ClusterAPI(25-40)ClusterAPIInstallerComponent(154-159)ClusterAPIInstallerComponentImage(162-169)ClusterAPIInstallerRevision(112-150)ClusterAPIList(177-186)ClusterAPISpec(43-65)ClusterAPIStatus(77-110)
operator/v1alpha1/zz_generated.deepcopy.go (1)
operator/v1alpha1/types_clusterapi.go (7)
ClusterAPI(25-40)ClusterAPISpec(43-65)ClusterAPIInstallerComponent(154-159)ClusterAPIInstallerComponentImage(162-169)ClusterAPIInstallerRevision(112-150)ClusterAPIList(177-186)ClusterAPIStatus(77-110)
🔇 Additional comments (30)
operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
1-23: Feature gate "ClusterAPIMachineManagement" is properly defined.The CRD manifest correctly references the feature gate, which is properly registered in
features/features.go(line 555) with appropriate scope (DevPreviewNoUpgrade, TechPreviewNoUpgrade) and annotation intypes_clusterapi.go(line 17). The structure and formatting are correct and follow the established pattern.operator/v1alpha1/register.go (1)
44-45: Registration follows Kubernetes conventions. BothClusterAPIandClusterAPIListtypes are properly defined and correctly registered. The types implement standard Kubernetes patterns withTypeMeta,ObjectMeta/ListMeta, and appropriate resource/list fields.openapi/openapi.json (2)
35322-35424: ClusterAPI schema definitions look structurally sound.The ClusterAPI, ClusterAPIList, ClusterAPISpec, and ClusterAPIStatus definitions follow Kubernetes conventions and include comprehensive validation rules. Field descriptions for
unmanagedCustomResourceDefinitions,activeConfigMaps, andtargetConfigMapsare detailed with DNS-1123 requirements and cardinality limits, which is appropriate. The use ofx-kubernetes-list-type: "set"and"atomic"is correct for their respective semantics.Since this appears to be auto-generated from source code, ensure that the Go type definitions in the operator/v1alpha1 package match this schema (required fields, field names, validation tags). Given that only this generated file is provided for review, the source code should be verified separately.
51623-51692: Info type definition: verify namespace and required-field defaults.The
io.k8s.apimachinery.pkg.version.Infotype defines versioning metadata with nine required fields and four optional fields. However, there's a potential inconsistency:
Namespace discrepancy: The AI summary references this as
io.k8s.apimachinery.pkg.apis.meta.v1.Info, but the schema showsio.k8s.apimachinery.pkg.version.Info. Confirm the correct namespace is intended and matches the source code.Required fields with defaults: All nine required fields have
"default": ""(empty string). For fields marked as required, having a default value is semantically unusual—clarify whether these should actually be optional or if the defaults should be removed.Optional field descriptions: Unlike Hunk 1, the descriptions for emulation and compatibility fields lack details about valid formats or constraints. Consider aligning documentation style if consistency is desired.
operator/v1alpha1/tests/clusterapis.operator.openshift.io/ClusterAPIMachineManagement.yaml (5)
1-82: LGTM!The
onCreatetests provide comprehensive coverage for ClusterAPI creation scenarios, including minimal creation, valid/invalidunmanagedCustomResourceDefinitionsentries, and DNS-1123 subdomain validation.
83-150: LGTM!The
onUpdatetests forunmanagedCustomResourceDefinitionscorrectly validate immutability constraints: items cannot be removed, the list cannot be unset once populated, but additions are permitted.
151-283: LGTM!The status subresource tests correctly validate revision uniqueness constraints (unique names, unique revision numbers) and the
desiredRevisionrequirement to reference the highest revision number.
284-378: LGTM!The
currentRevisionvalidation tests properly cover the constraint that it must reference an existing revision entry when set, while correctly allowing it to be absent.
379-459: LGTM!The revision immutability and deletion tests correctly validate that existing revisions cannot be modified but can be deleted. The test at lines 410-458 appropriately verifies that removing old revisions is permitted.
operator/v1alpha1/zz_generated.swagger_doc_generated.go (2)
138-166: LGTM!The SwaggerDoc entries for
ClusterAPI,ClusterAPIInstallerComponent, andClusterAPIInstallerComponentImageare consistent with their type definitions and follow the established pattern in this file.
179-207: LGTM!The SwaggerDoc entries for
ClusterAPIList,ClusterAPISpec, andClusterAPIStatusare well-documented and align with the field-level comments in the source type definitions.openapi/generated_openapi/zz_generated.openapi.go (6)
1194-1200: LGTM!The schema registrations are correctly formatted and follow the established naming conventions for the ClusterAPI v1alpha1 types.
60700-60748: LGTM!The ClusterAPI schema definition follows standard Kubernetes resource conventions with proper metadata, spec, and status fields.
60750-60771: LGTM!The ClusterAPIInstallerComponent schema correctly defines the required image field with proper type reference.
60869-60918: LGTM!The ClusterAPIList schema follows standard Kubernetes list resource conventions with proper metadata and items fields.
60920-60951: The source field already includes all required validation markers.The
UnmanagedCustomResourceDefinitionsfield inoperator/v1alpha1/types_clusterapi.go(lines 59-64) contains all four validation markers:
+kubebuilder:validation:MaxItems=128(line 60)+kubebuilder:validation:items:MaxLength=253(line 63)+kubebuilder:validation:items:XValidationfor DNS-1123 subdomain format (line 62)+kubebuilder:validation:XValidationfor immutability (line 61)These validations are not visible in the generated OpenAPI Go code because generated OpenAPI definitions don't serialize kubebuilder validation markers—they are processed separately by controller-gen to produce CRD manifests and validation rules.
Likely an incorrect or invalid review comment.
60953-61000: All validation constraints are properly enforced in the source type definition. Therevisionsfield inoperator/v1alpha1/types_clusterapi.goincludes:
+kubebuilder:validation:MaxItems=16+kubebuilder:validation:XValidationfor unique names, unique revision numbers, immutability, and revision number orderingThe generated OpenAPI schema does not include CEL validation rules because the OpenAPI 3.0 specification has limited support for such constraints—this is expected and correct behavior. The constraints are enforced at the API type level, which is what matters for validation.
Likely an incorrect or invalid review comment.
operator/v1alpha1/types_clusterapi.go (7)
1-7: LGTM!Package declaration and imports are appropriate for the ClusterAPI type definitions.
9-40: Well-structured ClusterAPI resource definition.The struct correctly implements singleton enforcement (line 24), feature gating (line 17), and proper immutability constraints (line 23) for the unmanagedCustomResourceDefinitions field. The use of a pointer for Spec and
omitzerofor Status follows the conventions discussed in prior reviews.
42-65: Excellent validation rules for the spec.The validation annotations correctly enforce:
- No duplicate CRD names (
listType=setat line 59)- Append-only semantics (line 61)
- Proper DNS-1123 subdomain format (line 62)
- Reasonable size limits (lines 60, 63)
The validation logic is sound and aligns with the documented use case.
67-110: Sophisticated and correct validation logic for status management.The CEL validation rules are well-crafted:
- Line 75 correctly ensures
desiredRevisionreferences the revision with the highest number- Line 76 ensures
currentRevisionconsistency with the revisions list- Lines 105-108 enforce critical invariants: unique names, unique numbers, monotonic increases, and immutability
These validations effectively prevent invalid state transitions in the revision management system.
112-150: Well-defined revision structure.The ClusterAPIInstallerRevision type properly defines:
- Required identification fields (Name, Revision, ContentID)
- Consistent validation for UnmanagedCustomResourceDefinitions matching the spec
- Ordered component list with
listType=atomic(line 146) - appropriate for installation orderingThe field constraints and validations are appropriate for the revision management use case.
152-169: Good extensibility design for component sources.The
MaxProperties=1constraint (line 153) on ClusterAPIInstallerComponent creates a union-like structure, enabling future addition of alternative component sources beyond images. The reuse ofmachineosconfigv1.ImageDigestFormatensures consistent validation across the OpenShift API.
171-186: LGTM!Standard Kubernetes list type implementation following established conventions.
operator/v1alpha1/zz_generated.deepcopy.go (6)
29-59: LGTM!The generated DeepCopy implementation for ClusterAPI correctly handles the pointer Spec field (lines 34-38) with proper nil checking and allocation, and properly deep copies ObjectMeta and Status.
61-92: LGTM!The deepcopy implementations for ClusterAPIInstallerComponent and ClusterAPIInstallerComponentImage correctly use simple assignment since these structs contain only value types (no pointers or slices requiring deep copies).
94-118: LGTM!The DeepCopyInto implementation for ClusterAPIInstallerRevision correctly handles both slice fields:
- UnmanagedCustomResourceDefinitions ([]string) with proper allocation and copy
- Components ([]ClusterAPIInstallerComponent) with proper allocation and copy
Nil checks are appropriately placed.
120-151: LGTM!The ClusterAPIList deepcopy implementation correctly iterates through Items and calls DeepCopyInto on each ClusterAPI element (lines 128-130), ensuring proper deep copying of the list contents.
153-172: LGTM!The ClusterAPISpec deepcopy correctly handles the UnmanagedCustomResourceDefinitions string slice with proper nil checking and allocation.
174-195: LGTM!The ClusterAPIStatus deepcopy correctly handles:
- Scalar fields (CurrentRevision, DesiredRevision) with simple assignment
- Revisions slice with proper iteration and nested DeepCopyInto calls (lines 180-182)
The implementation is correct and complete.
| func schema_openshift_api_operator_v1alpha1_ClusterAPIInstallerComponentImage(ref common.ReferenceCallback) common.OpenAPIDefinition { | ||
| return common.OpenAPIDefinition{ | ||
| Schema: spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "ClusterAPIInstallerComponentImage defines an image source for a component.", | ||
| Type: []string{"object"}, | ||
| Properties: map[string]spec.Schema{ | ||
| "digest": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "digest is an image reference to the image containing the component manifests. The reference must be a valid image digest reference in the format host[:port][/namespace]/name@sha256:<digest>. The digest must be 64 characters long, and consist only of lowercase hexadecimal characters, a-f and 0-9. The length of the field must be between 1 to 447 characters.", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| }, | ||
| Required: []string{"digest"}, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the source types file mentioned in the review comment
find . -name "types_clusterapi.go" -o -name "*clusterapi*" -type f | grep -E "types|clusterapi" | head -20Repository: openshift/api
Length of output: 96
🏁 Script executed:
# Search for ClusterAPIInstallerComponentImage type definition and its Digest field
rg -A 20 "type ClusterAPIInstallerComponentImage" --type goRepository: openshift/api
Length of output: 1979
🏁 Script executed:
# Search for ImageDigestFormat type definition
rg -B5 -A10 "type ImageDigestFormat" --type goRepository: openshift/api
Length of output: 2631
🏁 Script executed:
# Also search for where machineosconfigv1 is imported from
rg "machineosconfigv1" operator/v1alpha1/types_clusterapi.go | head -5Repository: openshift/api
Length of output: 196
The OpenAPI schema is missing validation properties that are defined in the source type.
The source type machineosconfigv1.ImageDigestFormat in machineconfiguration/v1/types_machineosconfig.go properly includes kubebuilder validation markers:
+kubebuilder:validation:MinLength=1+kubebuilder:validation:MaxLength=447- Two
+kubebuilder:validation:XValidationrules for format validation
However, the generated OpenAPI schema at lines 60773-60792 lacks the corresponding properties: minLength, maxLength, and pattern. This is a code generation issue where the validation markers are not being properly translated into the OpenAPI definition.
🤖 Prompt for AI Agents
In @openapi/generated_openapi/zz_generated.openapi.go around lines 60773 -
60792, The generated OpenAPI function
schema_openshift_api_operator_v1alpha1_ClusterAPIInstallerComponentImage is
missing the string validation constraints from
machineosconfigv1.ImageDigestFormat; update the "digest" property's SchemaProps
to include MinLength: 1, MaxLength: 447 and the appropriate Pattern (the regex
that enforces host[:port][/namespace]/name@sha256:<64-lowercase-hex> or at
minimum the @sha256:[a-f0-9]{64} portion used in the kubebuilder XValidation),
so the OpenAPI output matches the source kubebuilder markers and preserves the
same validation semantics.
| func schema_openshift_api_operator_v1alpha1_ClusterAPIInstallerRevision(ref common.ReferenceCallback) common.OpenAPIDefinition { | ||
| return common.OpenAPIDefinition{ | ||
| Schema: spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Type: []string{"object"}, | ||
| Properties: map[string]spec.Schema{ | ||
| "name": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "name is the name of a revision.", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| "revision": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "revision is a monotonically increasing number that is assigned to a revision.", | ||
| Type: []string{"integer"}, | ||
| Format: "int64", | ||
| }, | ||
| }, | ||
| "contentID": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "contentID uniquely identifies the content of this revision. The contentID must be between 1 and 255 characters long.", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| "unmanagedCustomResourceDefinitions": { | ||
| VendorExtensible: spec.VendorExtensible{ | ||
| Extensions: spec.Extensions{ | ||
| "x-kubernetes-list-type": "set", | ||
| }, | ||
| }, | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "unmanagedCustomResourceDefinitions is a list of the names of ClusterResourceDefinition (CRD) objects which are included in this revision, but which should not be installed or updated. If not set, all CRDs in the revision will be managed by the CAPI operator.", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: "", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "components": { | ||
| VendorExtensible: spec.VendorExtensible{ | ||
| Extensions: spec.Extensions{ | ||
| "x-kubernetes-list-type": "atomic", | ||
| }, | ||
| }, | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "components is list of components which will be installed by this revision. Components will be installed in the order they are listed.\n\nThe maximum number of components is 32.", | ||
| Type: []string{"array"}, | ||
| Items: &spec.SchemaOrArray{ | ||
| Schema: &spec.Schema{ | ||
| SchemaProps: spec.SchemaProps{ | ||
| Default: map[string]interface{}{}, | ||
| Ref: ref("github.com/openshift/api/operator/v1alpha1.ClusterAPIInstallerComponent"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Required: []string{"name", "revision", "contentID", "components"}, | ||
| }, | ||
| }, | ||
| Dependencies: []string{ | ||
| "github.com/openshift/api/operator/v1alpha1.ClusterAPIInstallerComponent"}, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the source type definitions for ClusterAPIInstallerRevision
# First, locate the source file
echo "=== Locating source type file ==="
fd -e go types_clusterapi | head -20
# Check contentID field with context
echo -e "\n=== Checking contentID field ==="
rg -A 5 -B 2 'ContentID' --type go -i | head -50
# Check components field with context
echo -e "\n=== Checking components field ==="
rg -A 5 -B 2 'Components' --type go -i | head -50Repository: openshift/api
Length of output: 8064
🏁 Script executed:
#!/bin/bash
# Search specifically for Components field in ClusterAPIInstallerRevision struct
echo "=== Searching for Components field with validation markers ==="
rg -A 10 'Components.*\[\]ClusterAPIInstallerComponent' operator/v1alpha1/types_clusterapi.go
echo -e "\n=== Looking at full struct context ==="
rg -B 20 'ContentID.*string' operator/v1alpha1/types_clusterapi.go | grep -A 25 'type ClusterAPIInstallerRevision'Repository: openshift/api
Length of output: 1230
Add missing maxItems validation marker for components field and investigate OpenAPI code generation for contentID constraints.
The source type definitions have incomplete validation markers:
-
componentsfield (operator/v1alpha1/types_clusterapi.go): The documentation states "The maximum number of components is 32", but the source field lacks the+kubebuilder:validation:MaxItems=32marker. The generated OpenAPI schema is missing the correspondingmaxItemsproperty. -
contentIDfield: While the source correctly includes+kubebuilder:validation:MinLength=1and+kubebuilder:validation:MaxLength=255markers, the generated OpenAPI schema (lines 60817-60819) is missing the correspondingminLengthandmaxLengthproperties. This suggests a code generation issue that should also be addressed.
| var map_ClusterAPIInstallerRevision = map[string]string{ | ||
| "name": "name is the name of a revision.", | ||
| "revision": "revision is a monotonically increasing number that is assigned to a revision.", | ||
| "contentID": "contentID uniquely identifies the content of this revision. The contentID must be between 1 and 255 characters long.", | ||
| "unmanagedCustomResourceDefinitions": "unmanagedCustomResourceDefinitions is a list of the names of ClusterResourceDefinition (CRD) objects which are included in this revision, but which should not be installed or updated. If not set, all CRDs in the revision will be managed by the CAPI operator.", | ||
| "components": "components is list of components which will be installed by this revision. Components will be installed in the order they are listed.\n\nThe maximum number of components is 32.", | ||
| } | ||
|
|
||
| func (ClusterAPIInstallerRevision) SwaggerDoc() map[string]string { | ||
| return map_ClusterAPIInstallerRevision | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ClusterAPIInstallerRevision has a type-level doc comment in the source
rg -B5 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Get more context before the type declaration
rg -B15 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.go | head -25Repository: openshift/api
Length of output: 1233
🏁 Script executed:
#!/bin/bash
# Check if other types have doc comments
rg -B3 'type ClusterAPI struct' operator/v1alpha1/types_clusterapi.go | head -10Repository: openshift/api
Length of output: 514
🏁 Script executed:
#!/bin/bash
# Check the ClusterAPIInstallerComponent type
rg -B3 'type ClusterAPIInstallerComponent struct' operator/v1alpha1/types_clusterapi.go | head -10Repository: openshift/api
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Check the generated swagger doc file to see pattern of empty string keys
rg -A8 'var map_ClusterAPI = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 756
🏁 Script executed:
#!/bin/bash
# Check ClusterAPIInstallerComponent map
rg -A8 'var map_ClusterAPIInstallerComponent = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Check ClusterAPIList map
rg -A8 'var map_ClusterAPIList = map\[string\]string' operator/v1alpha1/zz_generated.swagger_doc_generated.goRepository: openshift/api
Length of output: 651
🏁 Script executed:
#!/bin/bash
# Get the full ClusterAPIInstallerRevision type with all context
rg -A20 'type ClusterAPIInstallerRevision struct {' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Get the full ClusterAPIInstallerRevision type with all context (escaping braces)
rg -A20 'type ClusterAPIInstallerRevision struct' operator/v1alpha1/types_clusterapi.goRepository: openshift/api
Length of output: 915
Add type-level documentation comment for ClusterAPIInstallerRevision.
The map_ClusterAPIInstallerRevision in the generated swagger doc lacks an empty string key for type-level documentation, unlike all other type maps in this file (map_ClusterAPI, map_ClusterAPIInstallerComponent, map_ClusterAPIList, etc.). This inconsistency exists because the ClusterAPIInstallerRevision type in types_clusterapi.go is missing a doc comment.
Add a doc comment to the ClusterAPIInstallerRevision type definition in operator/v1alpha1/types_clusterapi.go. The auto-generated swagger doc file will be regenerated via hack/update-swagger-docs.sh to include the empty string key.
🤖 Prompt for AI Agents
In @operator/v1alpha1/zz_generated.swagger_doc_generated.go around lines 167 -
177, Add a type-level doc comment to the ClusterAPIInstallerRevision type
declaration in operator/v1alpha1/types_clusterapi.go so the swagger generator
emits the empty-string key; specifically, locate the type named
ClusterAPIInstallerRevision and add a brief comment line immediately above it
(e.g., "// ClusterAPIInstallerRevision ...") describing the type, then re-run
hack/update-swagger-docs.sh to regenerate zz_generated.swagger_doc_generated.go
which will include the "" key in map_ClusterAPIInstallerRevision.
|
@mdbooth: 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. |
4 participants
No description provided.