-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
generate CRD manifests and fix for oc explain #1485
generate CRD manifests and fix for oc explain #1485
Conversation
Generate validation for: containerruntimeconfig controllerconfig kubeletconfig machineconfig machineconfigpool Using update-codegen-crds target from https://github.com/openshift/build-machinery-go, which will be included in a separate commit as it is not directly usable with the MCO repo until coreos/ignition#917 is resolved. The current generation is done via hacking ignition's config/v2_2/types/schema.go to have dummy `json:",inline"` for fields that are causing errors. Some background on this, there were 2 attempts to add generated schemas in: openshift#1403 and openshift#955. This supercedes those with updated generation methods. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
Now that we have generated schemas, for oc explain to work we need to turn on validation via preserveUnknownFields: false in the spec. There is also one error in the autogenerated machineconfigpool spec due to IntOrString, see: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#intorstring Also minor formatting in machineconfig.crd.yaml. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@yuqi-zhang: This pull request references Bugzilla bug 1705750, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In 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/test-infra repository. |
/retest |
manifests/machineconfig.crd.yaml
Outdated
items: | ||
type: string | ||
homeDir: | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t support all these ignition fields. I think it’s only the ones we had that we allow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I will look to manually edit for now, unless you know of a better method we can source these MCO-specific use-cases.
awesome 👍 /approve will leave to the team to further take a look and address comments, well done! |
/retest |
1 similar comment
/retest |
Interesting error from e2e tests failure
|
/retest Will fix up the descriptions. In the meantime let's see if that was a flake |
As per review, added back descriptions and removed unused fields. Note that |
looks good, needs bindata update |
Manually modify machineconfig crd fields to have description fields again. Also remove unused fields. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
Run ./hack/update-generated-bindata.sh Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
bcf6e90
to
9f8b3ab
Compare
Re-gen'ed bindata and re-ordered commits a bit. Should be good to go. |
@yuqi-zhang: No Bugzilla bug is referenced in the title of this pull request. In 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/test-infra repository. |
Removing BZ as well since this is now targeting master |
/retest one throttling one api |
f4cc003
to
caf106b
Compare
Add nullable to additionalTrustBundle, cloudProviderCAData, proxy, and kernelArguments, as they will otherwise be generated as null during CI runs, and the validation will cause it to fail. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
caf106b
to
2bc87a8
Compare
Ok, I changed it to nullable instead of omitempty and it passed CI. Also note that there are potentially other fields that need to be nullable. Basically any field that can be null (and is not |
Ah, so one more thing, since in |
/test e2e-gcp-op |
I think its not a flake but a change in behaviour, fixing now |
This allows MCC/MCD to validate the ignition section instead, much like we've always done. Otherwise, any non-valid ignition section will instead get pruned by the validation. A bit more background: for oc explain to work, we need to have preserveUnknownFields: false which means all fields not defined by the crd schema will be pruned upon generation of the CR. Since we only define ignition sections that are allowed, an invalid ignition spec field will simply get removed when the machineconfig is created. Adding the flag tells validation that anything under Config is allowed, and we will let MCC/MCD do the validation instead, like we do today. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
Ok I added A bit more background: for oc explain to work, we need to have
which means all fields not defined by the crd schema will be pruned upon generation of the CR. Since we only define ignition sections that are allowed to be machineconfig sections, an invalid ignition spec field will simply get removed when the machineconfig is created. Adding the flag tells validation that anything under Config is allowed, and we will let MCC/MCD do the validation instead, like we do today. Now it may technically be more inline with the API to just let the fields be pruned, but I feel that disrupts existing user experience. And since we will be switching to RawExtention anyways, that will not be validate-able via the CRD validation, so we will eventually have to do this. |
/retest |
/approve amazing :) |
Nice work Jerry! |
As a final clarification, this adds schema and schema validation for the manifests that live within our repo. This will enable |
As per our conversation this sounds correct :) /LGTM |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ericavonb, kikisdeliveryservice, runcom, sinnykumari, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-4.4 |
@yuqi-zhang: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you. In 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/test-infra repository. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@yuqi-zhang: new pull request created: #1520 In 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/test-infra repository. |
Now with openshift/machine-config-operator#1485 oc explain is working for *.machineconfiguration CRDs. Also remove "mcoconfigs" as it does not seem to exist anywhere. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
There are 3 commits in this change that aims to add OpenAPIV3 validation and get
oc explain
to work for MCO repo CRDS. Note that this does NOT include the actual generator code, this that does not current work with the MCO repo manifests. Will open another PR with the generator and explain what needs to be fixed for that to work, but for now this should getoc explain
going.Commit 1:
Generate crd manifests with OpenAPIV3 schemas
Generate validation for:
containerruntimeconfig
controllerconfig
kubeletconfig
machineconfig
machineconfigpool
Using update-codegen-crds target from
https://github.com/openshift/build-machinery-go, which will be
included in a separate commit as it is not directly usable with the
MCO repo until coreos/ignition#917 is
resolved. The current generation is done via hacking ignition's
config/v2_2/types/schema.go to have dummy
json:",inline"
for fieldsthat are causing errors.
Some background on this, there were 2 attempts to add generated
schemas in: #1403
and #955.
This supercedes those with updated generation methods.
Commit 2:
Fix autogenerated crd schemas for oc explain
Now that we have generated schemas, for oc explain to work we need to
turn on validation via preserveUnknownFields: false in the spec.
There is also one error in the autogenerated machineconfigpool spec
due to IntOrString, see: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#intorstring
Also minor formatting in machineconfig.crd.yaml.
Commit 3:
update generated bindata after crd changes