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

Add conditional install for CA and Prod ClusterIssuer #60

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

iyannsch
Copy link
Contributor

Currently, the theia.cloud-base helmchart always installs three ClusterIssuer: issuerca, issuerprod, and issuerstaging. This might make sense for a testing environment or a cluster just serving Theia but might be unnecessary for those environments in which ClusterIssuers are already handled.
According to @jfaltermeier it would be useful to opt-out of automatic installs of issuerca, issuerprod. Fixes #50.

My changes introduce two new useExisting values that disable the creation of new ClusterIssuer. issuerstaging is always created as required for testing.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @iyannsch , thank you very much for the PR :)

Instead of using flag useExisting, I'd prefer using enable: We use this already in other parts of the helm charts to toggle installation and AFAIK it is quite common to use this in Helm charts.

With this, you can also simplify the if checks in the cluster issuers, e.g. to if .Values.issuerca.enable.

Please also increase the version counter after the next. in the version property in charts/theia.cloud-base/Chart.yaml.

Note that I'll be on vacation from 26.6 to 2.7. in case you push a change in the meantime. I can re-review afterwards :)

@iyannsch iyannsch requested a review from lucas-koehler June 26, 2024 05:29
@iyannsch
Copy link
Contributor Author

Thanks for you comment and feedback @lucas-koehler,

I addressed your changes and request a new review from your end :)

Please do enjoy your vacation, looking forward to hearing from you in July ☺️

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @iyannsch , thanks for the updates! I added some suggestions that simplify the if checks and enable the issuers by default. The latter resembles the previous behavior.

Apparently, conflicting changes have been merged in the meantime. Please rebase the changes :)

@iyannsch
Copy link
Contributor Author

iyannsch commented Jul 4, 2024

@lucas-koehler Thanks for your review!
I incorporated all your suggestions and rebased to ensure a clean merge :)

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Looks good to me now! Thanks for the updates and the rebase @iyannsch :)

@lucas-koehler lucas-koehler merged commit 33f323f into eclipse-theia:main Jul 5, 2024
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.

Cert Manager Handling
2 participants