-
Notifications
You must be signed in to change notification settings - Fork 11
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 generated CRDs directly available for testing and development #214
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.
Can you update the path to something like deploy/crds
as discussed in the earlier thread, I also think we should drop a README in that dir with something like:
cert-manager recommends the CRDs are deployed as part of the Helm chart, these CRDs exist for reference and development.
After this merges we can create an github issue to refactor it out the helm module.
Will do! Thanks for reviewing @ThatsMrTalbot! |
a8f5fef
to
9f789c1
Compare
@ThatsMrTalbot, your initial review comments should be addressed now. I have also created a WIP PR to verify/test that this would actually work in our projects: cert-manager/trust-manager#493. Rhere are probably additional (minor) details that need to be adjusted. PTAL! |
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.
A couple of little makefile things - what do you think? I don't feel super strongly but I think these might be worth updating unless I'm missing something!
81bad3d
to
6f67118
Compare
Signed-off-by: Erik Godding Boye <[email protected]>
@ThatsMrTalbot @inteon Are you able to review this again? All remarks should be addressed now. |
Thanks for making the changes! This has been discussed extensively in the bi-weekly so I'm happy to lgtm and approve this and we can create a issue to tidy up the logic into a more appropriate module later as we agreed. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ThatsMrTalbot 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 |
/unhold I reckon this is good to go! |
@erikgb it looks like the README is not auto-generated currently: cert-manager/approver-policy#545 |
Strange! I will take a look. |
This is a new attempt at what was started in #209, but accidentally got merged before being fully reviewed (and then reverted in #213).
I think the CRDs generated by controller-gen should be available in our projects, not just temporary files used as input to Helm CRD templates. While this will semi-duplicate the CRDs in project repositories, I think it has a lot of benefits (not limited to):
controller-gen
. It can currently generate both webhook configuration resources and RBAC (roles and cluster roles).I have suggested the default kubebuilder path for CRD manifests, even if we don't use/require kubebuilder in our projects. Open for other suggestions, but I think the term "bases" is important - as I think we can use kustomize to transform CRD in other use cases. When deciding on the path, please keep in mind that: