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

WIP: don't install CRDs of disabled subcharts #941

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MBrouns
Copy link

@MBrouns MBrouns commented Apr 16, 2024

Possible solution for #938

@MBrouns MBrouns force-pushed the disabled-subchart-crd-fix branch from 7a5e1b7 to 342b5e0 Compare April 16, 2024 16:17
Comment on lines +59 to +61
if err := helmchartutil.ProcessDependenciesWithMerge(chrt, vals); err != nil {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

If this actually works, we'll need it on upgrade too.

Copy link
Author

@MBrouns MBrouns Apr 17, 2024

Choose a reason for hiding this comment

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

That makes sense. I could probably add it to applyCRDs instead to cover all the use-cases, but that would mean changing the signature of that function to also include the vals. Is that better?

@souleb
Copy link
Member

souleb commented Apr 16, 2024

This results in helmchartutil.ProcessDependenciesWithMerge being called twice. We need a test case for idempotence here.

@MBrouns
Copy link
Author

MBrouns commented Apr 16, 2024

I can put in some time tomorrow or Thursday I think to write test cases and do it on upgrade as well. Any other concerns that need to be addressed?

@stefanprodan
Copy link
Member

stefanprodan commented Apr 16, 2024

We need a test case for idempotence here.

Reading this I hope we are safe:

In any case, if anywhere in that logic a map from the chart is touched, calling the function twice will mess up the whole thing, so we really need a test.

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.

3 participants