-
Notifications
You must be signed in to change notification settings - Fork 98
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
Update apiextensions to v1 #353
Conversation
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.
@k0da just to double-check, everything under chart/k8gb/templates/k8gb.absa.oss_gslbs_crd.yaml
is purely a result of generation, no manual modifications were required?
@ytsarev The flow is change controller-gen options to produce v1 into config/crd/bases/k8gb.absa.oss_gslbs.yaml, modify it to validate http and paths fields of an IngressSpec and copy it over into chart/k8gb/templates |
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.
@k0da Please make the generation and manual modification in form of separate commits so it is clear what is actually happening in the git history.
Apparently we also need to get rid of duplicate in deploy/crds/k8gb.absa.oss_gslbs_crd.yaml
Is it possible to symlink chart/k8gb/templates/k8gb.absa.oss_gslbs_crd.yaml
and config/crd/bases/k8gb.absa.oss_gslbs.yaml
to avoid manual copy over? (I'm not sure if it will properly work during packaging)
@somaritane surprise surprise, we hit the case when we need a Merge commit containing multiple logically separated commits ;)
@ytsarev yet it still looks like exceptional case rather than common one :) |
Reworked in a way:
|
Makefile
Outdated
@@ -410,7 +410,7 @@ define generate | |||
endef | |||
|
|||
define manifest | |||
$(call controller-gen,crd:trivialVersions=true rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases) | |||
$(call controller-gen,crd paths="./..." output:crd:artifacts:config=chart/k8gb/templates/) |
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.
is crd version v1 is the default in sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0
? I recall explicit version statement in the first version of this PR
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.
yes, it is. Since v1beta1 is deprecated quite a while ago. We can have it explicit though
* Switches to v1 crd by default * fix/cleanup generator opts (generator outputs crd only "output:crd:artifact") Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
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.
Apparently
deploy/crds/k8gb.absa.oss_gslbs_crd.yaml
still should be removed
Since controller isn't using webhook or cert-manger we don't really need to pass crd through kustomize in order to install it. We switch generating CRD into chart template directory. In order to keep single file reference. Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
After CRD generated, we need to bring back validation of .spec.ingress.rules.http field. Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
done |
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.
Probably last bit, sorry, just remembered.
We should remove associated kubelinter exclusion https://github.com/AbsaOSS/k8gb/blob/master/.kube-linter.yaml#L3
@ytsarev not yet. DNSEndpoint is still v1beta1 |
@k0da indeed 👍 |
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.
Looks great
DNSEndpoint addresed in kubernetes-sigs/external-dns#2001 |
This updates apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1
Signed-off-by: Dinar Valeev dinar.valeev@absa.africa