Vendor helm chart versions based on chartfile.yaml #420

craigfurman · 2020-11-06T15:19:12Z

When calling tk tool charts vendor, and a directory already exists at
the expected chart path, recursively delete that directory unless its
Chart.yaml matches the version of the chart specified in chartfile.yaml.

This allows vendored charts whose versions have changed to be downloaded
by tanka users without first removing the charts directory.

Return errors when Chart.yaml doesn't exist, or is malformed.

Implements #419

craigfurman · 2020-11-06T15:33:52Z

pkg/helm/charts.go

+				continue
+			} else {
+				log.Printf("Removing %s@%s", r.Chart, r.Version.String())
+				if err := os.RemoveAll(chartPath); err != nil {


RemoveAll is a bit scary. I'm reasonably happy that this code shouldn't delete anyone's entire project checkout (or home dir) unless they configure their chartfile directory to ".", or "../..", and that directory happens to contain a Chart.yaml. Perhaps it's still worth including a more stringent check? What do you reckon?

craigfurman · 2020-11-23T09:55:59Z

@sh0rez would you mind reviewing this implementation of #419, if you have time?

sh0rez

Nice work! Sorry it took so long on my end to review this. Feel free to ping me more frequently if this happens again.

I left a couple of comments on details, but I'm generally very happy with this :)

sh0rez · 2020-11-24T13:23:48Z

pkg/helm/charts.go

+			chartManifestPath := filepath.Join(chartPath, "Chart.yaml")
+			chartManifestBytes, err := ioutil.ReadFile(chartManifestPath)
+			if err != nil {
+				return fmt.Errorf("error reading chart manifest: %s", err)


Suggested change

return fmt.Errorf("error reading chart manifest: %s", err)

return fmt.Errorf("reading chart manifest: %w", err)

Let's omit error here, as this error might be wrapped further.

Also we generally use %w for errors (go 1.13 error wrapping)

This produces output like this:

Syncing Repositories ... Pulling Charts ... reading chart manifest: open /Users/craig/workspace/gitlab.com/gitlab-com/gl-infra/k8s-workloads/tanka-deployments/charts/memcached/Chart.yaml: no such file or directory

This error doesn't seem to be wrapped further. If we don't want to prepand "error" here, we could add wrapping higher up the call stack? That might duplicate the word "error" from other branches, if their libraries include that word in error messages though.

sh0rez · 2020-11-24T13:24:53Z

pkg/helm/charts.go

+			if err != nil {
+				return fmt.Errorf("error reading chart manifest: %s", err)
+			}
+			var chartManifest map[string]interface{}


Can we define a struct for this one? Makes unmarshalling and using the result safer

sh0rez · 2020-11-24T13:26:44Z

pkg/helm/charts.go

+			}
+			currentChartVersion, ok := chartManifest["version"]
+			if !ok {
+				return fmt.Errorf("chart manifest %s does not contain a version field", chartManifestPath)


Garbage in the charts folder would make us stop here, with on clear path to fix this.

Either we require manual user interaction (rm -r the folder, which we should suggest in that case), or we do it ourselves.

wdyt? (also /cc @sdboyer, you probably have an opinion here 😝)

there are 3 error branches here that deal with garbage in the charts folder. On the one hand, we could consider the charts folder to be fully managed by tanka, and do what we want with it. On the other hand, it's always safer to not delete something when you don't know what it is 🙂

I'm happy to defer to the maintainers on this.

this error branch is now removed as a result of implementing the above request to create a struct for chartManifest, but I think this conversation still applies to the other branches

When calling `tk tool charts vendor`, and a directory already exists at the expected chart path, recursively delete that directory unless its Chart.yaml matches the version of the chart specified in chartfile.yaml. This allows vendored charts whose versions have changed to be downloaded by tanka users without first removing the charts directory. Return errors when Chart.yaml doesn't exist, or is malformed. Signed-off-by: Craig Furman <craig.furman89@gmail.com>

craigfurman · 2020-11-24T14:26:47Z

Thanks for reviewing @sh0rez! I've implemented some of the suggestions and replied to another. Let me know what you think 👍

sh0rez

LGTM and sorry you were left without a response for so long.

I like the direction this takes a lot, and while we can surely make it smoother in the future (error messages, failure handling), this is a leap in the right direction. Thanks!

pkg/helm/charts.go

craigfurman · 2020-12-11T14:23:48Z

Thanks @sh0rez! Looks like there are some dangling threads after the merge. I could address them in a follow-on, or we could let them lie for now?

sh0rez · 2020-12-11T14:57:42Z

Sure, let's work on a follow up. I'd like to see this evolve more into the direction of a package manager eventually, where it gives strong guarantees on reproducibility. Luckily we don't need to resolve dependencies, as charts already include them. Also there are some rough edges we could address:

If Chart.yaml is absent, tk fails right now. This is unnecessary, because we could attempt recovery by re-downloading the respective Chart
If Chart.yaml is malformed, same applies as above
We need to get clear whether we claim absolute ownership over the charts directory. If we do so, we can happily delete garbage data and create a clean state each time. If not, it becomes much more complex, given we should not delete the users data. If we claim ownership, we should likely mark the folder as read-only.

These are just ideas, happy to discuss how to bring this further!

(Also let's continue this in a issue)

craigfurman mentioned this pull request Nov 6, 2020

tk tool charts vendor idempotence #419

Closed

craigfurman commented Nov 6, 2020

View reviewed changes

sh0rez requested changes Nov 24, 2020

View reviewed changes

craigfurman force-pushed the helm-chart-vendor-versioning-awareness branch from 05f1b12 to 71bf02f Compare November 24, 2020 14:25

craigfurman requested a review from sh0rez December 7, 2020 13:13

sh0rez approved these changes Dec 11, 2020

View reviewed changes

pkg/helm/charts.go Show resolved Hide resolved

sh0rez merged commit 5c286b7 into grafana:master Dec 11, 2020

craigfurman deleted the helm-chart-vendor-versioning-awareness branch December 11, 2020 14:22

Temikus mentioned this pull request Dec 30, 2020

Charttool issues if the directory already exists #454

Closed

craigfurman mentioned this pull request Feb 7, 2021

Remove helm repo update #507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor helm chart versions based on chartfile.yaml #420

Vendor helm chart versions based on chartfile.yaml #420

craigfurman commented Nov 6, 2020

craigfurman Nov 6, 2020

craigfurman commented Nov 23, 2020

sh0rez left a comment

sh0rez Nov 24, 2020

craigfurman Nov 24, 2020

sh0rez Nov 24, 2020

craigfurman Nov 24, 2020

sh0rez Nov 24, 2020

craigfurman Nov 24, 2020

craigfurman Nov 24, 2020

craigfurman commented Nov 24, 2020

sh0rez left a comment

craigfurman commented Dec 11, 2020

sh0rez commented Dec 11, 2020 •

edited

Loading

	return fmt.Errorf("error reading chart manifest: %s", err)
	return fmt.Errorf("reading chart manifest: %w", err)

Vendor helm chart versions based on chartfile.yaml #420

Vendor helm chart versions based on chartfile.yaml #420

Conversation

craigfurman commented Nov 6, 2020

Choose a reason for hiding this comment

craigfurman commented Nov 23, 2020

sh0rez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigfurman commented Nov 24, 2020

sh0rez left a comment

Choose a reason for hiding this comment

craigfurman commented Dec 11, 2020

sh0rez commented Dec 11, 2020 • edited Loading

sh0rez commented Dec 11, 2020 •

edited

Loading