feat: Parse HelmRelease with chartRef

[jfroy]

This patch adds minimal support for parsing HelmRelease objects with
a chartRef field instead of a chart. There is no support for
processing such releases in any way, but at least flux-local won't abort
during parsing.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 76.59574% with 11 lines in your changes missing coverage. Please review.

Project coverage is 93.26%. Comparing base (5409e97) to head (c015202).

Files	Patch %	Lines
flux_local/manifest.py	78.37%	8 Missing ⚠️
flux_local/helm.py	50.00%	2 Missing ⚠️
flux_local/tool/test.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #709      +/-   ##
==========================================
- Coverage   93.48%   93.26%   -0.22%     
==========================================
  Files          20       20              
  Lines        2165     2199      +34     
==========================================
+ Hits         2024     2051      +27     
- Misses        141      148       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jfroy]

I'll look at the failures in a few hours.

[jfroy]

I'm not sure what the CI failure is. "Resource not accessible by integration"?

[allenporter]

Thank you for this contribution!

I think the failure may be a permission issue trying to post the diff to the PR comment thread (likely something already broken before your change).

I'd love to see what it would take to get this feature fully working e.g. if its loading the chart from somewhere else, we may be able to make it work similar to how values from references work. Have you thought about what it would take?

Either way, will be happy to review in depth. (may take a few days)

[jfroy]

Thank you for this contribution!

I think the failure may be a permission issue trying to post the diff to the PR comment thread (likely something already broken before your change).

I'd love to see what it would take to get this feature fully working e.g. if its loading the chart from somewhere else, we may be able to make it work similar to how values from references work. Have you thought about what it would take?

Either way, will be happy to review in depth. (may take a few days)

Yeah I hope to follow up with more code to handle dereferencing the chartRef. For a HelmChart reference, most of the existing code will be reusable. For OCIRepository references, I'm not yet sure what that will look like. Maybe nothing more than parsing the OCIRepository and dealing with the various ref forms.

[allenporter]

I'd like to understand more about where this is headed. The reason is because of the change to make the chart option touched a lot of parts of the code. If we're just ignoring them, then I want to find sinpler ways to ignore them until I better understand the plan to add useful functionality for them.

So is the goal for now just to make flux local not fail?

[jfroy]

I'd like to understand more about where this is headed. The reason is because of the change to make the chart option touched a lot of parts of the code. If we're just ignoring them, then I want to find sinpler ways to ignore them until I better understand the plan to add useful functionality for them.

So is the goal for now just to make flux local not fail?

This specific PR is only to allow flux-local to not throw an exception parsing HelmRelease objects with a chartRef instead of a chart field, indeed. I tried to keep the changes to a minimum to accomplish this goal, while also satisfying mypy (e.g. dealing with chart being Optional). I think this PR puts flux-local in a good position for future work to fully support chartRef.

[allenporter]

Given that this adds no other functionality, then i'd like to find a way to make minimal code changes to do the filtering only. (essentially do it without changing the types to make the chart optional which is the part that requires many other changes)

Adding actual support for this feature might be a heavy lift, and if you don't intend to add full support then i'd rather not have half way support and have the complexity in the code of making the chart optional. Happy to talk about how to design full support for this (which may be similar to substituteFrom, but i'm not sure).

[jfroy]

I think I do want to implement this fully. I only did a rebase to avoid too much upstream drift. I will have some time later in the summer when I take some time off.

[allenporter]

OK, i'm happy to start with filtering then adding full support when ready.

[volschin]

I would really like to see a solution here, because using one chartRef makes flux-local in the way it is used also for diff in the cluster-template completely unusable.☹️

To better show you an example, what I‘m speaking of:
volschin/home-ops@8433109

[allenporter]

I would really like to see a solution here, because using one chartRef makes flux-local in the way it is used also for diff in the cluster-template completely unusable.☹️

To better show you an example, what I‘m speaking of:
volschin/home-ops@8433109

A solution I would accept is to filter them out with minimal code changes so the rest of the cluster still works, given we don't have a proposal to fully support.

[volschin]

At the moment I have commented out the full helmrelease part of the flux-diff workflow. A solution, that filters out the helmreleases with chartRef and run the others would be a good progress.👏

[allenporter]

Looking at this closer, I don't think we need to populate a HelmChart on the fly here rahther than dropping it and having a separate object for the chartRef. We can abstract this complexity HelmChart.

[allenporter]

I think i was able to get full support working in #769

[jfroy]

Thanks for working on it, I was just about to start looking into this again (finally on vacation!).