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

fix: Update cert-manager to use crds.enabled instead of the deprecated installCRDs option #443

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kinyat
Copy link

@kinyat kinyat commented Feb 21, 2025

What does this PR do?

This is a follow-up to #435.

Instead of changing the option directly, we add a pre-check before setting installCRDs.

If the user has already set the new crds.enabled flag in either values or set, we do not add the deprecated installCRDs option.

This helps prevent breaking changes and allows users to avoid the deprecated installCRDs option.

Update: Follow-up to #443 (review)

I have updated the PR to pump up the version of the cert-manager to the latest minor version.

I have also updated the PR to remove the usage of installCRDs and replaced it with both the new crds.enabled and crds.keep options.

Motivation

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I ran pre-commit run -a with this PR

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@kinyat kinyat requested a review from a team as a code owner February 21, 2025 02:14
@kinyat kinyat changed the title Update cert-manager to use crds.enabled instead of the deprecated installCRDs fix: update cert-manager to use crds.enabled instead of the deprecated installCRDs Feb 21, 2025
@kinyat kinyat changed the title fix: update cert-manager to use crds.enabled instead of the deprecated installCRDs fix: update cert-manager to use crds.enabled instead of the deprecated installCRDs option Feb 21, 2025
@kinyat kinyat changed the title fix: update cert-manager to use crds.enabled instead of the deprecated installCRDs option fix: Update cert-manager to use crds.enabled instead of the deprecated installCRDs option Feb 21, 2025
Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

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

Lets just bump up the version of cert-manager and set crds.enabled instead of installCRDs. We can break compatibility with older versions because we use semantic versioning on the module.

@kinyat
Copy link
Author

kinyat commented Feb 24, 2025

Lets just bump up the version of cert-manager and set crds.enabled instead of installCRDs. We can break compatibility with older versions because we use semantic versioning on the module.

I have updated the PR to pump up the version of the cert-manager to the latest minor version.
I have also updated the PR to remove the usage of installCRDs and replaced it with both the new crds.enabled and crds.keep options.

Can you please give it a 2nd look?

value = true
},
{
name = "crds.keep"
Copy link

@hgadham hgadham Feb 24, 2025

Choose a reason for hiding this comment

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

I don't recommend hardcoding this. I would suggest to remove this entirely and let helm values decide whether we need to install CRDs or not. This is no different to other ex: aws_load_balancer_controller.

Copy link

@hgadham hgadham Feb 24, 2025

Choose a reason for hiding this comment

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

Additionally, I should have the flexibility to choose whether to manage CRDs through the addon or deploy them externally. Would harcoding limits my option to disable CRD ? If it is preferred option, why are we not consistently doing it for other addons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cert Manager Currently Using deprecated option
3 participants