-
Notifications
You must be signed in to change notification settings - Fork 49
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
make it possible to run kubectl explain crd
#524
make it possible to run kubectl explain crd
#524
Conversation
output explain:
CRD output:
|
2fddf5f
to
5a87814
Compare
kubectl explain crd
5a87814
to
35618d9
Compare
kubectl explain crd
kubectl explain crd
getting
|
@@ -368,63 +368,198 @@ func GetClusterRole() *rbacv1.ClusterRole { | |||
return role | |||
} | |||
|
|||
func GetCrd() *extv1beta1.CustomResourceDefinition { | |||
crd := &extv1beta1.CustomResourceDefinition{ | |||
func GetCrd() *extv1.CustomResourceDefinition { |
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.
Don't we want to annotate the _types with descriptions and generate this with gen-openapi ?
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 existence of the /shared
folder in types cause an error with operator-sdk generate crds
command.
so, I created it in an older version for reference and then recreated it manually in components.go:GetCrd()
.
the resulting .crd file is identical to the reference (except for the version migration changes of course)
4e281ba
to
d7b29f8
Compare
73779c2
to
51557ba
Compare
updated crd and explain outputs |
Infra *Placement `json:"infra,omitempty"` | ||
// Workloads defines placement configuration for worker nodes | ||
Workloads *Placement `json:"workloads,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.
Placement struct is missing.
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.
Done
func GetCrd() *extv1beta1.CustomResourceDefinition { | ||
crd := &extv1beta1.CustomResourceDefinition{ | ||
func GetCrd() *extv1.CustomResourceDefinition { | ||
validationSchema := &extv1.CustomResourceValidation{ |
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.
Add we sure we cannot store at the container the stuff generated from operator-sdk and read it as golang struct ?
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 currently can't generate it, it doesn't know how to handle the shared folder:
cluster-network-addons-operator (add_csv_schema_validation_detail) $ ./build/_output/bin/operator-sdk generate crds
INFO[0000] Running CRD generator.
github.com/kubevirt/cluster-network-addons-operator/pkg/apis/networkaddonsoperator/shared:-: CRD for NetworkAddonsConfig.networkaddonsoperator.network.kubevirt.io has no storage version
FATA[0001] error generating CRDs from APIs in pkg/apis: error generating CRD manifests: error running CRD generator: not all generators ran successfully
plus, (correct me if I'm wrong, but) marshaling the automated yaml to struct will require us to always know the exact struct format. so in fact it could mean taking up the same amount of time.
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.
Well it's all about duplicity I see the description on structs and on the Schema, It has to be doable somehow we will se in the future if we do deeper integration of cnao with operator-sdk.
173867c
to
5b3a350
Compare
updated crd output |
/lgtm |
5b3a350
to
f917f7e
Compare
f917f7e
to
542e5c6
Compare
test/e2e/lifecycle/upgrade_test.go
Outdated
@@ -32,6 +33,7 @@ var _ = Context("Cluster Network Addons Operator", func() { | |||
BeforeEach(func() { | |||
UninstallRelease(oldRelease) | |||
InstallRelease(newRelease) | |||
CheckCrdExplainable() |
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.
Would it make sense to keep the second one in It
? Since it is really something we want to test, not a setup of the context
test/e2e/lifecycle/upgrade_test.go
Outdated
@@ -18,6 +18,7 @@ var _ = Context("Cluster Network Addons Operator", func() { | |||
Context(fmt.Sprintf("when operator in version %s is installed and supported spec configured", oldRelease.Version), func() { | |||
BeforeEach(func() { | |||
InstallRelease(oldRelease) | |||
CheckCrdExplainable() |
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.
Why do we care about this in before each?
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 idea was to check before and after the upgrade.
I agree this would better suited in a specific It
. tell me what you think
Done
/hold @qinqon is working on a PR to auto generate the crd via operator-sdk |
15b8146
to
c60d39e
Compare
both failed test don't recreate locally, retesting /retest |
/hold cancel We are not going to depend on auto generated CRDs since looks like we have some non tooling issues there. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qinqon 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 |
/hold |
a4ecb32
to
0639214
Compare
/retest |
crd explain fails since |
'operator-sdk generate crds' generates descriptions from the annotations added to 'networkaddonsconfig_types.go' This commit adds these annotations. Signed-off-by: Ram Lavi <[email protected]>
Currently there is a inconsistency between the manifest templator outputed crd manifest and the type annotations in scope field. Altough the manifests are controlled by the manifest generator lets fix the annotation avoid future confusion. Signed-off-by: Ram Lavi <[email protected]>
Let's add openAPIV3Schema detail for "kubectl explain crd" Since operator-sdk crd cannot run. this commit manually creates the openAPIV3Schema that should've been generated by operator-sdk crd API. Signed-off-by: Ram Lavi <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>
0639214
to
61715ae
Compare
/retest |
61715ae
to
523e9be
Compare
there is an issue where on upgrade of apiVersion from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1, oc explain does not show the CR documentation. This is solved in parallel PRs (SSP PR [1], CDI PR [2]) by adding PreserveUnknownFields=false to the CRD's spec [1] kubevirt/kubevirt-ssp-operator@7ecd297 [2] kubevirt/containerized-data-importer#1343 Signed-off-by: Ram Lavi <[email protected]>
523e9be
to
dcc4418
Compare
/hold awaiting response from kubernetes/kubernetes#95702 before merging |
/lgtm |
/hold cancel since currently the parameter is not deprecated we can move on with the PR |
What this PR does / why we need it:
Let's make make it possible to run
kubectl explain networkaddonsconfigs
add components annotations to types
operator-sdk generate crds
generates descriptions from the annotations added tonetworkaddonsconfig_types.go
This commit adds these annotations.
fix NetworkAddonsConfig scope to Cluster in api types
Currently there is a inconsistency between the manifest templator
outputed crd manifest and the type annotations in scope field.
Altough the manifests are controlled by the manifest generator
lets fix the annotation avoid future confusion.
add openAPIV3Schema validation parameters
Let's add openAPIV3Schema detail for "kubectl explain crd"
Since operator-sdk crd cannot run. this commit manually creates the openAPIV3Schema
that should've been generated by operator-sdk crd API.
adding test to make sure crd is explainable
Special notes for your reviewer:
PR depends on #537, #593 to be merged
Release note: