Allow defining a custom dir for each Helm Chart #706

julienduchesne · 2022-05-31T11:17:14Z

This allows a chartfile to contain multiple versions of the same chart.

Example:

directory: charts
repositories:
- name: flagger
  url: https://flagger.app
requires:
- chart: flagger/flagger
  directory: flagger-1.16.1
  version: 1.16.1
- chart: flagger/flagger
  directory: flagger-1.21.0
  version: 1.21.0
version: 1

This allows a chartfile to contain multiple versions of the same chart. Example: ```yaml directory: charts repositories: - name: flagger url: https://flagger.app requires: - chart: flagger/flagger directory: flagger-1.16.1 version: 1.16.1 - chart: flagger/flagger directory: flagger-1.21.0 version: 1.21.0 version: 1 ```

julienduchesne · 2022-05-31T11:18:43Z

pkg/helm/charts_test.go

@@ -40,18 +26,33 @@ func TestAddRepos(t *testing.T) {
 }

 func TestAdd(t *testing.T) {
-	c, err := InitChartfile(filepath.Join(t.TempDir(), Filename))
+	tempDir := t.TempDir()


The tests now depend on actual helm pulling the prometheus chart. Since the helm pull behavior is a bit unusual, I believe that a full test is better than mocking the Helm process

jvrplmlmn

Just a quick first round, will circle back later.

pkg/helm/helm.go

jvrplmlmn · 2022-05-31T12:55:46Z

pkg/helm/helm.go

+	// It is not possible to tell `helm pull` to extract to a specific directory
+	// so we extract to a temp dir and then move the files to the destination


This is my complete ignorance in regards with Helm, but would --untardir help for this purpose?

It does not. It just appends the directory and the untardir together and then also appends the chart's name. Which is completely weird IMO, untardir serves no purpose at all. And it's one of the reasons I decided to actually use Helm in the tests, it tests the unexpected behaviors of the helm CLI like this

jvrplmlmn · 2022-05-31T13:27:40Z

For completeness, I have checked out this branch and run a couple of manual tests, this is pretty neat 👍

✅ Vendoring 2 different versions of the same chart to 2 different locations:

tk tool charts init
tk tool charts add-repo flagger https://flagger.app
tk tool charts add flagger/[email protected]:flagger-1.16.1
tk tool charts add flagger/[email protected]:flagger-1.21.0
tk tool charts vendor

✅ Vendoring the same version of a chart to the default and a custom locations:

tk tool charts init
tk tool charts add-repo flagger https://flagger.app
tk tool charts add flagger/[email protected]
tk tool charts add flagger/[email protected]:flagger-1.16.1
tk tool charts vendor

julienduchesne · 2022-05-31T13:30:06Z

For completeness, I have checked out this branch and run a couple of manual tests, this is pretty neat 👍

✅ Vendoring 2 different versions of the same chart to 2 different locations:
tk tool charts init
tk tool charts add-repo flagger https://flagger.app
tk tool charts add flagger/[email protected]:flagger-1.16.1
tk tool charts add flagger/[email protected]:flagger-1.21.0
tk tool charts vendor
✅ Vendoring the same version of a chart to the default and a custom locations:
tk tool charts init
tk tool charts add-repo flagger https://flagger.app
tk tool charts add flagger/[email protected]
tk tool charts add flagger/[email protected]:flagger-1.16.1
tk tool charts vendor

👍
There's still the bug that you can vendor to the same directory twice, but that's pre-existing. I plan to fix that in another PR and then add a prune setting and we'll have a full reconciling command that we can call in CI to check if we're up-to-date

jvrplmlmn

A suggestion for pkg/helm/charts.go (lines 117 and 120 on the new diff):

log.Printf(" %s@%s exists", r.Chart, r.Version.String())
log.Printf("Removing %s@%s", r.Chart, r.Version.String())

Could we include there r.Directory?

Currently, on the second scenario described here: #706 (comment)

You get:

$ tk tool charts vendor
Pulling Charts ...
 flagger/[email protected] exists
 flagger/[email protected] exists

so is not clear to which one is referring to.

jvrplmlmn

LG2M

jvrplmlmn · 2022-06-01T10:54:54Z

pkg/helm/spec.go

@@ -64,6 +66,14 @@ type Requirement struct {
 	Directory string         `json:"directory,omitempty"`
 }

+func (r Requirement) String() string {
+	s := r.Chart + "@" + r.Version.String()
+	if r.Directory != "" {


Nit: We are building Requirement via parseReq, which already splits the repo/name from the version. If Requirement would have RepoName and ChartName instead of Chart, we could also infer here the default directory (is ChartName).

directory: charts repositories: - name: stable url: https://charts.helm.sh/stable - name: flagger url: https://flagger.app requires: - chart: flagger/flagger version: 1.16.1 version: 1

crates the chart under charts/flagger

You're right, also the call to helm show I was doing just to get the chart name was unnecessary. Thanks

The chart name is the second part of the chart (repo/name)

julienduchesne requested review from Duologic, sh0rez and a team May 31, 2022 11:17

julienduchesne commented May 31, 2022

View reviewed changes

julienduchesne force-pushed the julienduchesne/helm-dir branch 4 times, most recently from 0a2a1f6 to ba8b817 Compare May 31, 2022 11:34

julienduchesne removed request for a team, Duologic and sh0rez May 31, 2022 11:34

julienduchesne marked this pull request as draft May 31, 2022 11:34

julienduchesne force-pushed the julienduchesne/helm-dir branch 2 times, most recently from 4ae3fe5 to 6351ce4 Compare May 31, 2022 11:36

Use alpine for pipeline. Install helm

f4e66d3

julienduchesne force-pushed the julienduchesne/helm-dir branch from 6351ce4 to f4e66d3 Compare May 31, 2022 11:37

julienduchesne marked this pull request as ready for review May 31, 2022 11:38

julienduchesne requested review from sh0rez, a team and Duologic May 31, 2022 11:38

julienduchesne changed the title ~~Allow defining a custom dir for Helm Charts~~ Allow defining a custom dir for each Helm Chart May 31, 2022

jvrplmlmn reviewed May 31, 2022

View reviewed changes

Remove go:generate

600af08

jvrplmlmn reviewed May 31, 2022

View reviewed changes

Add directory to log messages

cc69c1f

julienduchesne requested a review from a team June 1, 2022 10:40

jvrplmlmn approved these changes Jun 1, 2022

View reviewed changes

Remove unneeded call to helm show

eb097cf

The chart name is the second part of the chart (repo/name)

julienduchesne merged commit 0428803 into main Jun 1, 2022

julienduchesne deleted the julienduchesne/helm-dir branch June 1, 2022 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow defining a custom dir for each Helm Chart #706

Allow defining a custom dir for each Helm Chart #706

julienduchesne commented May 31, 2022

julienduchesne May 31, 2022

jvrplmlmn left a comment

jvrplmlmn May 31, 2022

julienduchesne May 31, 2022 •

edited

Loading

jvrplmlmn commented May 31, 2022

julienduchesne commented May 31, 2022

jvrplmlmn left a comment

jvrplmlmn left a comment

jvrplmlmn Jun 1, 2022

julienduchesne Jun 1, 2022

		// It is not possible to tell `helm pull` to extract to a specific directory
		// so we extract to a temp dir and then move the files to the destination

Allow defining a custom dir for each Helm Chart #706

Allow defining a custom dir for each Helm Chart #706

Conversation

julienduchesne commented May 31, 2022

julienduchesne May 31, 2022

Choose a reason for hiding this comment

jvrplmlmn left a comment

Choose a reason for hiding this comment

jvrplmlmn May 31, 2022

Choose a reason for hiding this comment

julienduchesne May 31, 2022 • edited Loading

Choose a reason for hiding this comment

jvrplmlmn commented May 31, 2022

julienduchesne commented May 31, 2022

jvrplmlmn left a comment

Choose a reason for hiding this comment

jvrplmlmn left a comment

Choose a reason for hiding this comment

jvrplmlmn Jun 1, 2022

Choose a reason for hiding this comment

julienduchesne Jun 1, 2022

Choose a reason for hiding this comment

julienduchesne May 31, 2022 •

edited

Loading