fix: copy dependencies on aliasing to avoid sharing chart references on multiply aliased dependencies#9175
Conversation
|
@dastrobu Thank you for the PR, this is exactly what I need 👍 |
|
@itscaro thanks for the feedback. Just need to get the maintainers attention to get someone reviewing this PR. |
|
Bump! I'm also very interested on getting it merged. |
|
Bump! this would greatly simplify my charts 🙏 |
|
@bacongobbler you might have someone at least... well... look at this? |
|
Beg your pardon, @bacongobbler, but is there any chance this can be released as a patch version in the 3.7.x train, instead of waiting for 3.8.0? This bug makes it really hard for one to create reusable/composable charts... |
|
This sounds like a bugfix. We'll investigate soon but for now labeling as such and moving to next patch release milestone. |
|
would also like this one please as part of 3.8.2 |
|
|
||
| // empty dependencies and shallow copy all dependencies, otherwise parent info may be corrupted if | ||
| // there is more than one dependency aliasing this chart | ||
| out.SetDependencies() |
There was a problem hiding this comment.
I seems from my local testing and the usecase I have (attached in zip) this is not enough while the following changes from this other PR is working:
if md.Dependencies != nil {
dependencies := make([]*chart.Dependency, len(md.Dependencies))
for i := range md.Dependencies {
d := *md.Dependencies[i]
dependencies[i] = &d
}
md.Dependencies = dependencies
}There was a problem hiding this comment.
I am happy to look into this, when there is feedback from the maintainers what milestone is realistic for this PR. If there is a case I overlooked, I guess there should be another test case as well.
There was a problem hiding this comment.
@mederel since I had some spare time and these issues are relly quite annoying, I took a look at the issue you
mentioned.
The example from your zip boils down to the following graph:
graph TD;
umbrella-->child1;
umbrella-->child2;
umbrella-->child2-bis;
child2-bis-. alias .->child2;
child1-->common;
child2-->common;
child2-bis-->common;
The example can be a bit simplified by removing child1, while still showing the issue
graph TD;
umbrella-->child2;
umbrella-->child2-bis;
child2-bis-. alias .->child2;
child2-->common;
child2-bis-->common;
So we are essentially back to the test case from #9150, which is
graph TD;
parent-->foo;
parent-->bar;
foo-. alias .->child;
bar-. alias .->child;
foo-->grandchild;
bar-->grandchild;
The only notably difference is the use of import-values in the two child charts and this is where it gets interesting.
The issue reported in #9150 was related to information stored in
Line 55 in ced54b1
which is why I copied the
Chart objects.
The issue you found is similar but in a different place:
Line 47 in ced54b1
which is why you copied the
Dependency objects in the metadata.
So in the end we need both. I updated this PR with a fix and a test case for the issue you mentioned.
I have to say that I don't like the solution. There seems to be a conceptual issue with aliases in Helm's dependency
management implementation from my point of view.
Some parts of the implementation seem to assume to reflect alias with the following graph in memory:
graph TD;
parent-->child;
parent-- aliased -->child;
while others assume
graph TD;
parent-->child;
parent-- aliased --> child-copy;
Depending on which path to follow information such as "imported values" or "parent" must be stored on the edges of the graph (first diagram) or can be stored on the nodes (second diagram).
Anyway, there is no easy and nice way to fix this, but I would like to see a refactoring of the logic for Helm 4.
|
are Helm maintainers dead ? How many follower does it take to get on the top of the stack to be reviewed ? |
|
Your commitment is impressive, would love to see this merged :) |
What would be the next step? |
Hi @dastrobu are you able to resolve the conflicts in this PR so we can get this merged? I also pinged you in k8s slack |
…ltiply aliased dependencies Dependencies keep a reference on their parent chart, which breaks if a chart reference is shared among multiple aliases. By copying the dependencies, parent information can be set correctly to render the templates as expected later on. Note that this change will make ChartFullPath return a different path for sub-subcharts. It will contain the alias names instead of the path to the chart files which makes it consistent with paths to templates on the subchart level. Closes helm#9150 Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>
7342072 to
26d73c5
Compare
imported values are stored in dependency objects, which breaks if a chart dependency is shared among multiple aliases. By copying the dependency objects in the metadata values can be imported correctly. Supersedes helm#10174 Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>
26d73c5 to
b183ecc
Compare
|
@scottrigby thanks for coming back to this. I rebased and updated the PR and resolved the issues. It should be ready for review now. |
scottrigby
left a comment
There was a problem hiding this comment.
@dastrobu thanks for your contribution and persistence.
manually tested, has unit tests, and code review LGTM 👍
|
Removed @dastrobu are you able to make a v3 backport PR with |
|
@scottrigby thanks for the explanation, I did miss that helm 4 is already under development. Find the back port for helm 3 in #30802. |
|
Thanks everyone... We've been tracking this PR for 2 years. Great see it merged |
Dependencies keep a reference on their parent chart, which breaks if a chart reference is shared among multiple aliases.
By copying the dependencies, parent information can be set correctly to render the templates as expected later on.
Note that this change will make ChartFullPath return a different path for sub-subcharts. It will contain the alias names instead of the path to the chart files which makes it consistent with paths to templates on the subchart level.
Closes #9150
Signed-off-by: Daniel Strobusch 1847260+dastrobu@users.noreply.github.com
If applicable: