Generate the CRD with v1 (not v1alpha1) #17
Conversation
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Since this changes your resource to include a status subresource, have you checked that you are handling updates to the status using a status client within the controller? If you aren't, your controller would stop being able to update the status once this is merged
Makefile
Outdated
| CONTROLLER_GEN ?= go run vendor/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go | ||
| # Produce CRDs that work back to Kubernetes 1.11 (no version conversion) | ||
| CRD_OPTIONS ?= "crd:trivialVersions=true" | ||
| CRD_OPTIONS ?= "crd:trivialVersions=true,crdVersions=v1" |
There was a problem hiding this comment.
Unfortunately, we need to keep this consistent with what is in the machine-api-operator for the time being. https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_04_metal3provisioning.crd.yaml#L1 shows that is still using v1beta1.
I think that means deferring the fix for this until later in the 4.7 cycle, when we have completed the work to remove the metal3 management logic from the MAO. Unless we also need to backport the fix to 4.6, in which case I would expect a patch in the MAO repository to land before we make the change here.
There was a problem hiding this comment.
I think the point here is to copy this into the MAO, since the explain output is currently broken in 4.6.
There was a problem hiding this comment.
I believe it will be safe to copy what is currently generated in this PR over to MAO within the 4.6 timeline, can a PR raised for that and attached to the same bug?
There was a problem hiding this comment.
This generated CRD is different from the one in MAO specifically around https://github.com/openshift/machine-api-operator/blob/master/install/0000_30_machine-api-operator_04_metal3provisioning.crd.yaml#L98-L101. Either we have to update our code here to generate the enum or make the changes in MAO independent of these changes.
There was a problem hiding this comment.
The enum you've highlighted appears to be present in the version in this PR as well?
There was a problem hiding this comment.
Ok, so it should be safe to copy this generated file over to MAO and make sure they're synced?
api/v1alpha1/provisioning_types.go
Outdated
|
|
||
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:resource:path=provisionings,scope=Cluster | ||
| // +kubebuilder:subresource:status |
There was a problem hiding this comment.
These changes should be OK, since they match the MAO version.
@JoelSpeed by status client in the controller, do you mean the ClusterOperator tracking the status of the Operator? Moreover, this PR is changing the order of the kubebuilder comments and not adding a new one. Should this result in a status change somewhere? |
|
@sadasu If you look at the generated CRD in this repo, it doesn't currently include the status subresource, so this generation is adding the status subresource to the CRD in the eyes of the API server. However, if you look in the machine-api-operator repo, you can see the subresource is already there in that version, so as long as that is the one people are using, there's no breaking changes or anything here and my previous comment can be ignored |
asalkeld
force-pushed
the
crd-v1beta1-is-deprecated
branch
from
3346984 to
2c5c0e6
Compare
asalkeld
changed the title
openshift-ci-robot
added
do-not-merge/work-in-progress
asalkeld
force-pushed
the
crd-v1beta1-is-deprecated
branch
from
2c5c0e6 to
fcb2f02
Compare
openshift-ci-robot
removed
the
needs-rebase
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
asalkeld
force-pushed
the
crd-v1beta1-is-deprecated
branch
from
fcb2f02 to
4f456ae
Compare
asalkeld
force-pushed
the
crd-v1beta1-is-deprecated
branch
from
4f456ae to
f38ec83
Compare
|
Comparing the CRD file in this PR with the version currently in the MAO show no differences. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asalkeld, dhellmann 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-robot
added
the
approved
Well, it does show differences if I compare the right files. :head-desk: The file in this PR looks correct. |
|
/lgtm |
8 participants
Originally done to get "oc explain Provisioning" to print more information.
(https://bugzilla.redhat.com/show_bug.cgi?id=1880787)
Update:
So I think we should merge this, just because v1alpha1 is deprecated..
However this is not helping 'oc explain Provisioning'.
if I "oc apply -f manifests/0000_30_cluster-baremetal-operator_00_metal3provisioning.crd.yaml"
then 'oc explain Provisioning', I still get the same response.