-
Notifications
You must be signed in to change notification settings - Fork 22
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
(testing): Update testdata generation and testing logic #4
(testing): Update testdata generation and testing logic #4
Conversation
Signed-off-by: Bryce Palmer <[email protected]>
Signed-off-by: Bryce Palmer <[email protected]>
) | ||
|
||
// InstallCertManagerBundle installs CertManager onto a Kubernetes cluster | ||
func InstallCertManagerBundle(hasv1beta1CRs bool, kubectl kubernetes.Kubectl) error { |
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.
Do we need the legacy versions what support v1beta1 CRDs? I believe we need not even test again such old k8s cluster versions.
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.
This is a good point. I think a lot of this was copied over from a previous PR against the operator-sdk that, at the time, did test v1beta1 CRDs.
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.
Updated this to no longer have a bool for if there are v1beta1 CRs, but left the legacy versions in place for now. I'm happy to remove the legacy version support all together but felt that warranted at least a bit of conversation (although I do agree it's unlikely we will be testing against kube <= 1.16 so I'm +1 on removing)
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.
After a sync meeting it was decided to eventually try and push these changes upstream to Kubebuilder. With that in mind I would lean towards keeping this as is for now and making further improvements when contributing this to Kubebuilder.
|
||
// AllowProjectBeMultiGroup will update the PROJECT file with the information to allow we scaffold | ||
// apis with different groups. be available. | ||
func AllowProjectBeMultiGroup(sample sample.Sample) error { |
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 should be having a separate test/scaffolding for multi group. Can't we set this in the init command itself?
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.
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 advanced molecule testdata sample enables multi-group support. I don't see why we couldn't set this in the init command itself, but I don't recall if the sample generator currently allows for setting custom flags in the init subcommand at the moment.
Signed-off-by: Bryce Palmer <[email protected]>
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.
Did a quick review and it looks good. I'm not looking in detail at the test library implementation, have a view suggestions on it, but we can look at it later. No need to block this PR for that!
Nice work! :)
/lgtm
/approve
Description of the change:
Motivation for the change:
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs