Skip to content
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

Separate helm stuff from pure (/reusable) crd definitions #926

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

jkremser
Copy link
Member

Fixes #925

Using helm's $files.Get to include those two CRDs based on the k8gb.deployCrds flag. This way we can have pure CRDs that are also deployable w/o helm (or remotely referencing the github url)

side note: there is also a line with $$(( $(CLUSTERS_NUMBER) + 1 )), because when doing full local setup, we are actually deploying n+1 clusters (+1 for the edge dns cluster)

Signed-off-by: Jirka Kremser [email protected]

Copy link
Collaborator

@kuritka kuritka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks 👍 !

Please check my review comment

@@ -1,4 +1,3 @@
{{- if .Values.k8gb.deployCrds }}
---
Copy link
Collaborator

@kuritka kuritka Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does --- needs to be here ? When brackets were used my controller-gen move it unintentionally. I mean, it looks unnecessary and controllergen can move it again in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a mistake in my previous version of the PR.. it was appending the crds for k8gb to that file, now it's only > instead of >>.

--- at the beginning is what controller-gen crd:crdVersions=v1 paths="./..." output:crd:stdout actually produces, I am not sure what you mean by "controllergen can move it" (perhaps it was the issue with append instead of rewrite logic?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkremser , thx for clarification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the review, good catch actually 👍

@jkremser jkremser merged commit 0c09888 into k8gb-io:master Jul 21, 2022
@jkremser jkremser deleted the crd-template branch July 21, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRD + helm
2 participants