Add an option which builds recursively the dependent charts by ccojocar · Pull Request #4468 · helm/helm

ccojocar · 2018-08-14T15:45:05Z

This is based on PR #2278.

The recursive build is now implemented iteratively and also some tests are added.

It was also tested manually on a more complex chart https://github.com/ccojocar/environment-scalewalnut-staging/tree/master/env.

fixes #2247

cc @jstrachan

ccojocar · 2018-08-14T15:49:23Z

cc @technosophos @thomastaylor312

thomastaylor312 · 2018-08-16T17:23:56Z

Hey @ccojocar. Thanks for the PR! I'll try to take a look at this tomorrow

merqurio · 2018-08-18T09:49:22Z

Is there something I can do to help this PR make it into 2.11.0 ? I was going to implement this feature when I found the PR. Thanks @ccojocar 👍

Maybe adding some information about this feature on Complex Charts with Many Dependencies section might be useful for others looking into this kind of charts that might need recursive builds.

merqurio · 2018-08-21T18:48:08Z

Tested and worked like a charm. Thanks @ccojocar

thomastaylor312

The logic here is looking pretty good! I just have one concern about a breaking change of the API

thomastaylor312 · 2018-08-22T21:39:18Z

pkg/downloader/manager.go

 //
 // If SkipUpdate is set, this will not update the repository.
-func (m *Manager) Build() error {
+func (m *Manager) Build() ([]*chartutil.Dependency, error) {


This is breaking a publicly exposed API. Anyway you can change this to avoid the breaking change?

thomastaylor312 · 2018-08-22T21:43:32Z

pkg/downloader/manager.go

+		return err
+	}
+
+	m.SkipUpdate = true


Nit: I would recommend adding a comment here for anyone else who edits this code in the future about why SkipUpdate is set to true

I would also set SkipUpdate back to its original value at the end of this function. I am afraid of weird side effects down the road

ccojocar · 2018-08-23T07:56:13Z

@thomastaylor312 Thanks for review! I addressed all the issues. Please have a look again. Thanks

thomastaylor312 · 2018-08-27T19:05:30Z

@ccojocar So I was doing some manual validation on this and it wouldn't work for me. I used the nginx chart found in docs/examples and added a requirements file to it that looked like this:

dependencies:
    - name: redis
      version: "3.6.4"
      repository: "@stable"
    - name: alpine
      version: "0.1.0"
      repository: "file://../alpine"

This includes the alpine chart from the same directory, where I had also added a requirements file with a chart from the stable repo. This failed with the following output:

Hang tight while we grab the latest from your chart repositories...
...Unable to get an update from the "local" chart repository (http://127.0.0.1:8879/charts):
	Get http://127.0.0.1:8879/charts/index.yaml: dial tcp 127.0.0.1:8879: connect: connection refused
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 2 charts
Downloading redis from repo https://kubernetes-charts.storage.googleapis.com
Unpacking: charts/redis-3.6.4.tgz
Deleting outdated charts
No requirements found in charts/redis/charts.
Error: could not find charts/alpine: stat charts/alpine: no such file or directory

It looks like the downloadAll function is for some reason not unpacking the alpine tarball. Let me know if I wasn't clear with any of the reproduction steps here

ccojocar · 2018-08-29T09:27:10Z

This includes the alpine chart from the same directory, where I had also added a requirements file with a chart from the stable repo. This failed with the following output:

@thomastaylor312 Thanks for catching the issue. It is fixed now. The dependencies are also resolved for charts which are downloaded from a local repository.

ccojocar · 2018-08-29T10:14:41Z

/check-cla

merqurio · 2018-09-04T16:04:11Z

I think I found a bug, but I'm not completely sure. I have a complex chart, with a dependency that i want to disable.

├── Chart.yaml
├── charts
│   ├── subchart
│   │   ├── Chart.yaml
│   │   ├── charts
…
│   │   │   ├── subsubchart        <-- The chart I want to disable
│   │   │   │   ├── Chart.yaml

Although my values.yaml in the root chart, disables the subsubchart, the resources are rendered anyway, even if the computed values (helm install --debug …) are the expected ones.

subchart:
  values: …
  subsubchart:
    enabled: false

I'm happy to share more details if you think this might be a bug.

bacongobbler · 2018-09-04T20:12:09Z

@merqurio I think that could be related to #4490 but that bug may be irrelevant to this PR in particular. Am I right with that assumption?

bacongobbler · 2018-09-05T22:18:04Z

After doing a helm dep build --recursive it looks like both the packed and unpacked version of the chart is in the charts/ directory. Would you mind taking a look into that?

><> cat requirements.yaml
dependencies:
  - name: wordpress
    version: "2.1.10"
    repository: "@stable"
  - name: alpine
    version: "0.1.0"
    repository: "file://../alpine"

><> ../../../bin/helm dep build --recursive
Hang tight while we grab the latest from your chart repositories...
...Unable to get an update from the "local" chart repository (http://127.0.0.1:8879/charts):
        Get http://127.0.0.1:8879/charts/index.yaml: dial tcp 127.0.0.1:8879: connect: connection refused
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 2 charts
Downloading wordpress from repo https://kubernetes-charts.storage.googleapis.com
Unpacking: charts/wordpress-2.1.10.tgz
Unpacking: charts/alpine-0.1.0.tgz
Deleting outdated charts
Saving 1 charts
Downloading mariadb from repo https://kubernetes-charts.storage.googleapis.com/
Unpacking: charts/wordpress/charts/mariadb-4.3.1.tgz
Deleting outdated charts
No requirements found in charts/alpine/charts.
No requirements found in charts/wordpress/charts/mariadb/charts.

><> ls -p charts/
alpine/  alpine-0.1.0.tgz  wordpress/  wordpress-2.1.10.tgz

I also notice that the lockfile only shows child dependencies but not subchart dependencies. Is that intentional?

bacongobbler · 2018-09-05T22:22:01Z

pkg/downloader/manager.go

 			dep.Version = ver
+			if m.Recursive {
+				fmt.Fprintln(m.Out, "Unpacking:", archiveFile)
+				err := chartutil.ExpandFile(destPath, archiveFile)


this is probably where we'd want to os.Remove the archiveFile here.

This actually poses another interesting design question though: if the design of helm dep build is to store the tarballed charts in charts/, wouldn't the user also expect the same from helm dep build --recursive? If so, would it be convenient if we parsed the tree, but laid it out as a flat file structure like

><> ls -p charts/ alpine-0.1.0.tgz wordpress-2.1.0.tgz mariadb-4.3.1.tgz

That way, it'd be simpler overall to manage these resources in the umbrella chart, as you could reference mariadb configuration as --set mariadb.foo=bar. It would also mean that there's only one version of any given chart as part of the dependency tree rather than subtree A installing mariadb 4.3.1 and subtree B installing mariadb 4.4.0.

What do you think?

I think, it's a good idea to flatten the tree but then there is the limitation which you just described.

Maybe after building the tree, it can be traversed again to delete all the source files and just keep the Charts with the archive files. Something like:

alpine-0.1.0.tgz charts/ wordpress-2.1.0.tgz mariadb-4.3.1.tgz

bacongobbler · 2018-09-07T19:23:32Z

due to ongoing discussion/design questions around this feature, we're going to bump this off the 2.11 milestone.

merqurio · 2018-09-23T02:48:07Z

@merqurio I think that could be related to #4490 but that bug may be irrelevant to this PR in particular. Am I right with that assumption?

You were right about that @bacongobbler ! thanks for the tip

ccojocar · 2018-09-27T08:43:30Z

I also notice that the lockfile only shows child dependencies but not subchart dependencies. Is that intentional?

@bacongobbler It's not. Isn't enough to have the first level of dependencies in the lockfile? They will have their own lockfile anyhow. I don't think, it would make a difference to keep the entire dependency tree in the lockfile.

merqurio · 2018-10-24T22:27:17Z

@ccojocar @thomastaylor312 a friendly ping to don't let this stale. May I help somehow, please let me know.

ccojocar · 2018-10-25T07:18:18Z

@thomastaylor312 @merqurio @bacongobbler Please could you clarify which issue is preventing to get this merged? Thanks

ccojocar · 2019-02-24T19:09:31Z

@bacongobbler @thomastaylor312 I pushed a fix to keep only the packed dependencies in the charts folder as suggested in your comment. Also now the dependencies tree is flatten.

The lock file right now keeps track only of the child dependencies. Should we add the entire dependencies tree? I think, this is not really required because they can be deduced when traversing the tree starting from the first dependencies level, if each of these dependencies has a lock file.

bacongobbler · 2019-03-01T19:02:14Z

Should we add the entire dependencies tree? I think, this is not really required because they can be deduced when traversing the tree starting from the first dependencies level, if each of these dependencies has a lock file.

Yes. The entire point of a lockfile is to lock down what the package manager determined as the right dependency tree at a particular point in time. In our case, it was the last time the user ran helm dep up. Lockfiles are not intended to be used as a dependency resolver. That is the job of requirements.yaml.

An example use case:

Chart A has a lockfile that depends on Charts B and C.
Chart B has a lockfile that depends on Chart D, pinned to v1.1.0.
Chart C has a lockfile that also depends on Chart D, but it's pinned to v1.2.0.

When I call helm dep build on chart A, how can it resolve the dependency tree for Chart D? Each lockfile is pinned to a particular version so there's no way Helm could perform the dependency resolution necessary to target the right version.

Therefore, once you call helm dep up, you need to store the entire dependency tree in Chart A's lockfile. That way, helm dep build can rebuild the dependency tree. It cannot perform dependency resolution between two conflicting dependency trees.

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

…he recursive build and also add some comments Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

… dependencies tree Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

allen-servedio · 2019-03-13T12:31:24Z

It looks like @thomastaylor312 had requested changes - are these now resolved and can this be released for the community to use? Thank you all for your efforts with this!

ccojocar · 2019-03-24T10:50:45Z

Therefore, once you call helm dep up, you need to store the entire dependency tree in Chart A's lockfile. That way, helm dep build can rebuild the dependency tree. It cannot perform dependency resolution between two conflicting dependency trees.

@bacongobbler After looking at the install code, I am a bit confused. It seems that only helm dep build loads the dependencies from lockfile. The helm install --dep-up calls the Manager.Update method which reads the dependencies only from requirements file.

I think, the entire dependency tree has to be also updated in the requirements file of Chart A, otherwise the install will update only the direct dependencies of Chart A.

The current implementation is downloading all charts from dependency tree, without reflecting this flatten hierarchy in the requirements file.

What is your opinion on this?

kvolkovich-sc · 2019-04-19T13:16:47Z

Any updates? Really must-have feature.

ccojocar · 2019-05-03T08:35:42Z

I am wait for feedback to be able to rework the PR.

ccojocar · 2019-05-16T07:56:08Z

Closing this due to lack of feedback from Helm's maintainers. It seems to be stuck in this state since quite a long time. Thanks

omerxx · 2019-05-16T08:08:32Z

@bacongobbler @thomastaylor312 @merqurio
Can you guys please assist? This is a feature many of us are waiting for.
Please help @ccojocar so this can be reopened and worked on!
If you need any help please let us know

DASXCE · 2021-08-26T13:05:09Z

Waiting for this too... The workaround to manually(or to automate) helm dep update does not work with GitOps tools like ArgoCD.

zenntrix · 2021-10-27T15:40:22Z

Prod, can we bring this back to life and consider it or at least get a reason why it can't be merged?

samuellvicente · 2024-09-06T16:42:31Z

Adding interest on this issue, is there a change it might actually be merged?

thomastaylor312 self-requested a review August 16, 2018 17:24

thomastaylor312 added feature in progress size/medium labels Aug 22, 2018

thomastaylor312 suggested changes Aug 22, 2018

View reviewed changes

thomastaylor312 added the breaking label Aug 22, 2018

thomastaylor312 removed the breaking label Aug 24, 2018

bacongobbler added this to the 2.11.0 - Features milestone Aug 24, 2018

merqurio mentioned this pull request Aug 28, 2018

[WIP]: Kong ingress controller helm/charts#7350

Closed

ccojocar force-pushed the recursive-build branch from 8be3d43 to 1904353 Compare August 29, 2018 09:23

ccojocar force-pushed the recursive-build branch from 1904353 to d15db06 Compare August 29, 2018 09:29

bacongobbler reviewed Sep 5, 2018

View reviewed changes

bacongobbler removed this from the 2.11.0 - Features milestone Sep 7, 2018

michelleN mentioned this pull request Jan 10, 2019

Recursive dependency build option #2278

Closed

helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2019

ccojocar force-pushed the recursive-build branch from 5790469 to a99adb5 Compare February 24, 2019 18:56

helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2019

ccojocar added 6 commits March 5, 2019 10:25

Add an option which builds recursively the dependent chart

3ca69e7

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Restore the state of the skipUpdate flag to the sate before running t…

364b4e0

…he recursive build and also add some comments Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Keep the old Build signature in order to not break the public API

ce46173

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Unpack also the charts downloaded from a local repository

bb4445f

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Store only the packed charts in the charts folder when traversing the…

a04aae9

… dependencies tree Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

Fix the dependency recursive build test to use the flatten packed charts

5ff384b

Signed-off-by: Cosmin Cojocar <cosmin.cojocar@gmx.ch>

ccojocar force-pushed the recursive-build branch from a99adb5 to 5ff384b Compare March 5, 2019 08:25

helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2019

bacongobbler removed the size/medium label Mar 5, 2019

ccojocar closed this May 16, 2019

bacongobbler mentioned this pull request Aug 14, 2019

Recursive dependency handling for charts. #2247

Open

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

Conversation

ccojocar commented Aug 14, 2018

Uh oh!

ccojocar commented Aug 14, 2018

Uh oh!

thomastaylor312 commented Aug 16, 2018

Uh oh!

merqurio commented Aug 18, 2018

Uh oh!

merqurio commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomastaylor312 left a comment

Choose a reason for hiding this comment

Uh oh!

thomastaylor312 Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

thomastaylor312 Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

thomastaylor312 Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

ccojocar commented Aug 23, 2018

Uh oh!

thomastaylor312 commented Aug 27, 2018

Uh oh!

ccojocar commented Aug 29, 2018

Uh oh!

ccojocar commented Aug 29, 2018

Uh oh!

merqurio commented Sep 4, 2018

Uh oh!

bacongobbler commented Sep 4, 2018

Uh oh!

bacongobbler commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bacongobbler Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccojocar Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

bacongobbler commented Sep 7, 2018

Uh oh!

merqurio commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccojocar commented Sep 27, 2018

Uh oh!

merqurio commented Oct 24, 2018

Uh oh!

ccojocar commented Oct 25, 2018

Uh oh!

ccojocar commented Feb 24, 2019

Uh oh!

bacongobbler commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allen-servedio commented Mar 13, 2019

Uh oh!

ccojocar commented Mar 24, 2019

Uh oh!

kvolkovich-sc commented Apr 19, 2019

Uh oh!

ccojocar commented May 3, 2019

Uh oh!

ccojocar commented May 16, 2019

Uh oh!

omerxx commented May 16, 2019

Uh oh!

DASXCE commented Aug 26, 2021

Uh oh!

zenntrix commented Oct 27, 2021

Uh oh!

samuellvicente commented Sep 6, 2024

Uh oh!

merqurio commented Aug 21, 2018 •

edited

Loading

bacongobbler commented Sep 5, 2018 •

edited

Loading

bacongobbler Sep 5, 2018 •

edited

Loading

merqurio commented Sep 23, 2018 •

edited

Loading

bacongobbler commented Mar 1, 2019 •

edited

Loading