Vendor helm chart versions based on chartfile.yaml #420
Conversation
| continue | ||
| } else { | ||
| log.Printf("Removing %s@%s", r.Chart, r.Version.String()) | ||
| if err := os.RemoveAll(chartPath); err != nil { |
There was a problem hiding this comment.
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?
pkg/helm/charts.go
Outdated
| chartManifestPath := filepath.Join(chartPath, "Chart.yaml") | ||
| chartManifestBytes, err := ioutil.ReadFile(chartManifestPath) | ||
| if err != nil { | ||
| return fmt.Errorf("error reading chart manifest: %s", err) |
There was a problem hiding this comment.
| 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)
There was a problem hiding this comment.
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.
pkg/helm/charts.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("error reading chart manifest: %s", err) | ||
| } | ||
| var chartManifest map[string]interface{} |
There was a problem hiding this comment.
Can we define a struct for this one? Makes unmarshalling and using the result safer
pkg/helm/charts.go
Outdated
| } | ||
| currentChartVersion, ok := chartManifest["version"] | ||
| if !ok { | ||
| return fmt.Errorf("chart manifest %s does not contain a version field", chartManifestPath) |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
craigfurman
force-pushed
the
helm-chart-vendor-versioning-awareness
branch
from
05f1b12 to
71bf02f
Compare
|
Thanks for reviewing @sh0rez! I've implemented some of the suggestions and replied to another. Let me know what you think 👍 |
|
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? |
|
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:
These are just ideas, happy to discuss how to bring this further! (Also let's continue this in a issue) |
When calling
tk tool charts vendor, and a directory already exists atthe 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