Adopt controller-gen to generate crd manifests and adopt helm v3 way of managing crds #227
Conversation
jiayushe
force-pushed
the
master
branch
4 times, most recently
from
8350b24 to
5cf9870
Compare
|
In the crd generated, all the optional params of the spec are made as required fields. So not able to deploy zookeeper with exitsing helm charts |
…of managing crds Signed-off-by: She Jiayu <[email protected]>
|
Hi @anishakj , thanks for reviewing. Whether a param is optional or required is inferred from the JSON tag in the definition. Since they are optional, I have tagged them as optional in the main code. The generated CRD will not mark them as required now. The test should be passing as well. |
Codecov Report
@@ Coverage Diff @@
## master #227 +/- ##
==========================================
- Coverage 83.60% 81.81% -1.80%
==========================================
Files 11 11
Lines 1220 1248 +28
==========================================
+ Hits 1020 1021 +1
- Misses 137 162 +25
- Partials 63 65 +2
Continue to review full report at Codecov.
|
| ## Whether to create the CRD. | ||
| crd: | ||
| create: true | ||
|
|
||
| ## Specifies which namespace(s) the Operator should watch over. |
There was a problem hiding this comment.
Why this option is removed. Instead we can add in crd yaml
{{- if .Values.crd.create }}
There was a problem hiding this comment.
Thanks for reviewing. I have added back the option.
Ref: https://helm.sh/docs/chart_template_guide/accessing_files/
There was a problem hiding this comment.
other than this minor comment this pr LGTM, Thanks @jiayushe for doing this.
| @@ -188,8 +188,19 @@ func (s *ZookeeperClusterSpec) withDefaults(z *ZookeeperCluster) (changed bool) | |||
| } | |||
|
|
|||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | |||
| // ZookeeperCluster is the Schema for the zookeeperclusters API | |||
| // +k8s:openapi-gen=true | |||
There was a problem hiding this comment.
can we add a comment here specifying below makers comments are for generating crd using kubebuilder ( as we use operator-sdk framework for other things I feel it would be better to mention that)
There was a problem hiding this comment.
Done. Thanks for reviewing.
charts/zookeeper-operator/crds/zookeeper.pravega.io_zookeeperclusters.yaml
Show resolved
Hide resolved
| @@ -1,47 +1,6 @@ | |||
| {{- if .Values.crd.create }} | |||
There was a problem hiding this comment.
This file addition is unnecessary. I feel u should revert this to your previous change and remove the option for user to specify whether the crd needs to be created from the values.yaml. As per my understanding from this PR :
If a CRD is already found in the cluster, Helm will skip installing that CRD and will log this to the debug log. It will neither error out nor see if there are any differences between its version and the existing version.
There was a problem hiding this comment.
Okay I will remove it then. I was thinking that this could be used for backward compatibility with helm v2.
There was a problem hiding this comment.
We have migrated to helm 3 now, and don't plan to support helm 2
Change log description
Adopt
controller-gento generate crd manifests and adopt helm v3 way of managing crdsPurpose of the change
What the code does
READMEandMakefilepkg/apis/zookeeper/v1beta1/zz_generated.deepcopy.gousingoperator-sdk generate k8scontroller-gento generate crd manifests, this prevents inconsistency of crd manifests in manual and helm deploymentsHow to verify it
Run
make manifeststo generate crd manifests indeploy/crdsandcharts/zookeeper-operator/crds