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

kubevirt: Add ability to enable/disable hooks #161

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

Conversation

koendelaat
Copy link

I think the hardened helm charts provided in this repository are a great addition.

Currently we deploy these helm charts using ArgoCD, which for various reasons doesn't work well with the various hooks.

Although the hooks are well engineered, in some scenarios they are not required and might even cause problems (e.g. when used in combination with ArgoCD).

My proposal is add an ability to disable these hooks if required depending on the deployment mechanism.

Currently only modified the charts for kubevirt and cdi, but mechanism can be extended to other charts.

Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

Hi! Firstly, thanks for your contribution. We really appreciate those.

However, I must note that manually populating the charts/ directory is not how we work within this repository. Long story short, we modify the specific chart that we want to produce a new version for at packages/ and then use the Makefile commands to generate the final chart, its tarball, as well as include it into the static webpage.

We also ensure that commits are split between the changes to the respective package and the output of the different make targets e.g. #152. This makes it simple to review the changes.

With that aside, I saw that we basically added the options to disable said hooks in the values file. While I'm not completely against it, those ensure that the charts are properly upgraded and deleted and I'm hesitant to make the change without discussing all the options first.

I don't have experience with ArgoCD - is there a specific concern that we could perhaps work around?

Verified

This commit was signed with the committer’s verified signature.
koendelaat Koen de Laat
Signed-off-by: Koen de Laat <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
koendelaat Koen de Laat
Signed-off-by: Koen de Laat <[email protected]>
make html

Verified

This commit was signed with the committer’s verified signature.
koendelaat Koen de Laat
Signed-off-by: Koen de Laat <[email protected]>
@koendelaat koendelaat force-pushed the chore/optional_hooks branch from 34b5364 to 275eedf Compare October 17, 2024 16:24
@hardys
Copy link
Contributor

hardys commented Oct 25, 2024

This seems reasonable to me, thanks for the contribution @koendelaat!

As mentioned by @atanasdinov it would be helpful to understand the details around why you need to disable the hooks, but we have seen similar issues elsewhere, for example the rancher-turtles chart in this repo has hooks which assume Rancher is already running, which means they can fail in unexpected ways when charts are installed via the RKE2 helm controller (which does not provide any way to express ordering/dependencies between charts).

If we can capture some details here it will help users in future to decide if/when these new variables should be used, but otherwise lgtm.

@hardys hardys changed the title Add ability to enable/disable hooks kubevirt: Add ability to enable/disable hooks Oct 25, 2024
@hardys hardys requested a review from atanasdinov November 5, 2024 14:38
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.

None yet

3 participants