Repository: helm/helm
Pull request #7009 (2019-11-19 01:29:34)

fix(*): Helm v3 handling of APIVersion v1 charts dependencies
This fixes unexpected changes in APIVersion1 (Helm v2) Charts when they are operated on by Helm v3 using `package` or `dependency update`.

The fixes:
- `requirements.yaml` was being unconditionally merged into `Chart.yaml`, which produced packages which failed `helm lint`.
- `requirements.lock` was being converted into `Chart.lock` by `helm dep up` on an APIVersion1 chart.
- The code that loads `requirements.lock` and `requirements.yaml` in APIVersion1 charts for backwards compatibility, was incorrectly excluding those files from `helm package` and the `Files` collection in templates. As well as `Files` behaviour changing, this also broke sub-chart aliases for Helm v2 as those are stored in `requirements.yaml`.

Helm v3 incorrectly reads aliases from `Chart.yaml`'s `dependencies` map in APIVersion1 charts, but I didn't change that, as `helm lint` warns you that this is an unexpected combination already, and it's *as well as* `requirements.yaml` so doesn't break anything.

The changes in the third commit (ae74c91) are possibly not go-idiomatic, and I specifically welcome feedback or suggested improvements.

Fixes: #6974 

TBBle: I intend to fix the code-coverage failure, but not immediately. Hopefully later this week, pending other workload.
TBBle: I haven't added tests to cover the APIVersion v1 behaviours; the changes were visible in the test runs as they broke the dependency tests which were testing APIVersion v2 behaviours with a generated APIVersion v1 chart. Instead I changed those tests to test the behaviour of APIVersion v2 charts, and they pass again.
TBBle: Is there any kind of process to go through to have a PR reviewed? This's been sitting idle for a couple of weeks now, and I'd love to know if there's more I need to do.
hickeyma: @TBBle Thanks for the details. Some additional feedback below.

> requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.

I am unable to reproduce this. Can you give more details about it?

> The changes in the third commit (ae74c91) are possibly not go-idiomatic, and I specifically welcome feedback or suggested improvements.

Added feedback in https://github.com/helm/helm/pull/7009#discussion_r355539730
TBBle: Why should `requirements.lock` be excluded from packaging? It was included in packages generated by Helm v2.
TBBle: > > requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.
> 
> I am unable to reproduce this. Can you give more details about it?

With Helm v2.16.2:
```powershell
helm create testchart
```

Add a requirements.yaml:
```yaml
dependencies:
  - name: magic-ip-address
    version: ">0"
    repository: "@stable"
```

With Helm v2.16.2:
```powershell
helm dependency update testchart
```

With Helm v3.0.0 or v3.0.1
```powershell
helm package testchart
helm lint testchart-0.1.0.tgz
```

This will reproduce the failure:
```
==> Linting .\testchart-0.1.0.tgz
[INFO] Chart.yaml: icon is recommended
[ERROR] Chart.yaml: dependencies are not valid in the Chart file with apiVersion 'v1'. They are valid in apiVersion 'v2'   
Error: 1 chart(s) linted, 1 chart(s) failed
```
hickeyma: > Why should requirements.lock be excluded from packaging of APIVersion1 charts? It was included in packages generated by Helm v2, reconfirmed with Helm v2.16.2 with the same setup as the following repro case

I agree that it is included in Helm 2 but the `lock` file is not included in Helm 3. Is it still needed in the package as you will have the dependencies included? I would probably like to mirror Helm 3 in this regard.
hickeyma: > requirements.yaml was being unconditionally merged into Chart.yaml, which produced packages which failed helm lint.

Thanks for clarification of this in https://github.com/helm/helm/pull/7009#issuecomment-563989554.  I missed the subtly of doing update first with Helm 2.
TBBle: > > Why should requirements.lock be excluded from packaging of APIVersion1 charts? It was included in packages generated by Helm v2, reconfirmed with Helm v2.16.2 with the same setup as the following repro case

> I agree that it is included in Helm 2 but the `lock` file is not included in Helm 3. Is it still needed in the package as you will have the dependencies included? I would probably like to mirror Helm 3 in this regard.

Personally, I'd keep it around for API stability reasons, in case someone's scripts are using `.Files` to extract the real subchart versions from `requirements.lock` or similar. It seems a reasonably-arbitrary API break for `v1` to suddenly disappear the file.

Patch:
diff --git a/OWNERS b/OWNERS
index 715c8993d..d7dac5514 100644
--- a/OWNERS
+++ b/OWNERS
@@ -4,6 +4,7 @@ maintainers:
   - fibonacci1729
   - hickeyma
   - jdolitsky
+  - marckhouzam
   - mattfarina
   - michelleN
   - prydonius
diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go
index 3c38519bc..dd4fd2dff 100644
--- a/pkg/chart/loader/load.go
+++ b/pkg/chart/loader/load.go
@@ -114,12 +114,18 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			if err := yaml.Unmarshal(f.Data, c.Metadata); err != nil {
 				return c, errors.Wrap(err, "cannot load requirements.yaml")
 			}
+			if c.Metadata.APIVersion == chart.APIVersionV1 {
+				c.Files = append(c.Files, &chart.File{Name: f.Name, Data: f.Data})
+			}
 		// Deprecated: requirements.lock is deprecated use Chart.lock.
 		case f.Name == "requirements.lock":
 			c.Lock = new(chart.Lock)
 			if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil {
 				return c, errors.Wrap(err, "cannot load requirements.lock")
 			}
+			if c.Metadata.APIVersion == chart.APIVersionV1 {
+				c.Files = append(c.Files, &chart.File{Name: f.Name, Data: f.Data})
+			}
 
 		case strings.HasPrefix(f.Name, "templates/"):
 			c.Templates = append(c.Templates, &chart.File{Name: f.Name, Data: f.Data})
diff --git a/pkg/chartutil/chartfile.go b/pkg/chartutil/chartfile.go
index 68176ed5d..756b87cfb 100644
--- a/pkg/chartutil/chartfile.go
+++ b/pkg/chartutil/chartfile.go
@@ -42,7 +42,16 @@ func LoadChartfile(filename string) (*chart.Metadata, error) {
 //
 // 'filename' should be the complete path and filename ('foo/Chart.yaml')
 func SaveChartfile(filename string, cf *chart.Metadata) error {
+	// Pull out the dependencies of a v1 Chart, since there's no way
+	// to tell the serialiser to skip a field for just this use case
+	savedDependencies := cf.Dependencies
+	if cf.APIVersion == chart.APIVersionV1 {
+		cf.Dependencies = nil
+	}
 	out, err := yaml.Marshal(cf)
+	if cf.APIVersion == chart.APIVersionV1 {
+		cf.Dependencies = savedDependencies
+	}
 	if err != nil {
 		return err
 	}
diff --git a/pkg/chartutil/save.go b/pkg/chartutil/save.go
index bc8f5bdd9..e264c4391 100644
--- a/pkg/chartutil/save.go
+++ b/pkg/chartutil/save.go
@@ -141,8 +141,17 @@ func Save(c *chart.Chart, outDir string) (string, error) {
 func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error {
 	base := filepath.Join(prefix, c.Name())
 
+	// Pull out the dependencies of a v1 Chart, since there's no way
+	// to tell the serialiser to skip a field for just this use case
+	savedDependencies := c.Metadata.Dependencies
+	if c.Metadata.APIVersion == chart.APIVersionV1 {
+		c.Metadata.Dependencies = nil
+	}
 	// Save Chart.yaml
 	cdata, err := yaml.Marshal(c.Metadata)
+	if c.Metadata.APIVersion == chart.APIVersionV1 {
+		c.Metadata.Dependencies = savedDependencies
+	}
 	if err != nil {
 		return err
 	}
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index 81dd53614..e46af6944 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -163,7 +163,7 @@ func (m *Manager) Update() error {
 	}
 
 	// Finally, we need to write the lockfile.
-	return writeLock(m.ChartPath, lock)
+	return writeLock(m.ChartPath, lock, c.Metadata.APIVersion == chart.APIVersionV1)
 }
 
 func (m *Manager) loadChartDir() (*chart.Chart, error) {
@@ -634,12 +634,16 @@ func (m *Manager) loadChartRepositories() (map[string]*repo.ChartRepository, err
 }
 
 // writeLock writes a lockfile to disk
-func writeLock(chartpath string, lock *chart.Lock) error {
+func writeLock(chartpath string, lock *chart.Lock, legacyLockfile bool) error {
 	data, err := yaml.Marshal(lock)
 	if err != nil {
 		return err
 	}
-	dest := filepath.Join(chartpath, "Chart.lock")
+	lockfileName := "Chart.lock"
+	if legacyLockfile {
+		lockfileName = "requirements.lock"
+	}
+	dest := filepath.Join(chartpath, lockfileName)
 	return ioutil.WriteFile(dest, data, 0644)
 }
 


Test patch:
diff --git a/cmd/helm/dependency_update_test.go b/cmd/helm/dependency_update_test.go
index 53d67ef59..9afc04f8d 100644
--- a/cmd/helm/dependency_update_test.go
+++ b/cmd/helm/dependency_update_test.go
@@ -182,7 +182,7 @@ func TestDependencyUpdateCmd_DontDeleteOldChartsOnError(t *testing.T) {
 func createTestingMetadata(name, baseURL string) *chart.Chart {
 	return &chart.Chart{
 		Metadata: &chart.Metadata{
-			APIVersion: chart.APIVersionV1,
+			APIVersion: chart.APIVersionV2,
 			Name:       name,
 			Version:    "1.2.3",
 			Dependencies: []*chart.Dependency{
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index dd83c3dc2..ea235c13f 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -211,7 +211,7 @@ func TestUpdateBeforeBuild(t *testing.T) {
 		Metadata: &chart.Metadata{
 			Name:       "with-dependency",
 			Version:    "0.1.0",
-			APIVersion: "v1",
+			APIVersion: "v2",
 			Dependencies: []*chart.Dependency{{
 				Name:       d.Metadata.Name,
 				Version:    ">=0.1.0",
@@ -285,7 +285,7 @@ func checkBuildWithOptionalFields(t *testing.T, chartName string, dep chart.Depe
 		Metadata: &chart.Metadata{
 			Name:         chartName,
 			Version:      "0.1.0",
-			APIVersion:   "v1",
+			APIVersion:   "v2",
 			Dependencies: []*chart.Dependency{&dep},
 		},
 	}