-
Notifications
You must be signed in to change notification settings - Fork 409
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 schema #955
Generate CRD schema #955
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: damemi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @vrutkovs |
In 6591658, these vendored types were causing failures in the generator due to lack of JSON tags. I don't know what they are, but adding these tags in allowed the generator to run. |
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.
bindata need to be updated, please run hack/update-generated-bindata.sh
@damemi: PR needs rebase. 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. |
Node | ||
DirectoryEmbedded1 | ||
Node `json:"node,omitempty"` | ||
DirectoryEmbedded1 `json:"directoryEmbedded1,omitempty"` | ||
} |
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 might want to add these json tags to the upstream types as well.
/cc @runcom @ajeddeloh
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.
No, this should be dropped here. It's using struct embedding, the Node/DirectoryEmbedded1 should be invisible to anyone looking at the serialized json.
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.
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.
Note: if we're going to expose ign.Config as RawExtension externally, we won't need this. Will be discussed in Tuesday's RHCOS cabal.
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.
@LorbusChris that PR had the CRD validation manually written because the CRD generator was not working against this repo. This PR is just showing the steps to get that generator working. One of the errors was due to these types, which I'm assuming are some kind of dependency, not having JSON tags.
The fact that I've added these tags here is really more of a proof of concept (showing the CRD generator properly running). Due to my lack of knowledge on these or the upstream types I was hoping people more familiar with them could give more review, and determine if it's a change worth having.
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.
The Igntition types vendored here are generated from a json schema. We use struct embedding (which is what our json schema->go types generator outputs) to cut down a bit on code duplication.
It's also worth noting that Ignition types have their own validation code which can be used.
@damemi: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
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>
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>
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>
This adds the markers to the
machineconfiguration.openshift.io
doc.go to label the group appropriately eg, for use with CRD generators