Repository: helm/helm (go)

Issue:
Helm 3.0.0-rc.2 dependency build does not allow aliases
With helm 3, `helm dependency update` followed by `helm dependency build` complains about `Chart.lock` being out of sync when a repository contains an alias.

```
$ helm version
version.BuildInfo{Version:"v3.0.0-rc.2", GitCommit:"82ea5aa774661cc6557cb57293571d06f94aff0c", GitTreeState:"clean", GoVersion:"go1.13.3"}
$ helm repo list
NAME    	URL
stable  	https://kubernetes-charts.storage.googleapis.com/
jetstack	https://charts.jetstack.io
$ cat Chart.yaml
name: alias
apiVersion: v2
version: 1.0.0

dependencies:
  - name: postgresql
    version: "6.5.5"
    repository: "@stable"
$ helm dependency update .
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading postgresql from repo https://kubernetes-charts.storage.googleapis.com/
Deleting outdated charts
$ cat Chart.lock
dependencies:
- name: postgresql
  repository: https://kubernetes-charts.storage.googleapis.com/
  version: 6.5.5
digest: sha256:72394d61ae09dba4eb534de7c51a9285c2fe4ea6eb537c1593d593f06894ca68
generated: "2019-11-02T18:03:00.63001711+01:00"
$ helm dependency build .
Error: Chart.lock is out of sync with Chart.yaml
```

This is surprising since I just did the `update` to make sure the `Chart.lock` is in sync. If I resolve the alias, then everything works as expected.

```
$ cat Chart.yaml
name: alias
apiVersion: v2
version: 1.0.0

dependencies:
  - name: postgresql
    version: "6.5.5"
    repository: https://kubernetes-charts.storage.googleapis.com/
$ helm dependency update .
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading postgresql from repo https://kubernetes-charts.storage.googleapis.com/
Deleting outdated charts
$ helm dependency build .
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "jetstack" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading postgresql from repo https://kubernetes-charts.storage.googleapis.com/
Deleting outdated charts
```

Are aliases not supported in this context? If so, I think the error messages should be improved :)

Pull request:
fix(pkg/downloader): Resolve repo alias before checking digests on build
Fixes #6871 
Caused by #6736 

## What this PR does / why we need it:

`Update()` gets repo names before resolving a lock file by calling `resolveRepoNames(req)`. But that method changes aliased repo URLs into the actual URLs. That makes digests from `helm update` and `helm build` be different for each other.

To make them in sync, setting actual (resolved) repo URLs into the loaded chart during `helm build` is necessary. Thus, this commit adds an extra step in the `Build()` implementation.

For comments, this commit also changes the name of `getRepoNames()` into `resolveRepoNames()` to avoid misunderstanding since getters are expected to not mutate its input data in general.

~~Also, like `Update()`, `Build()` is changed to be finished immediately if there is no dependency.~~
(Edited: remove this feature since nil chart deps & non-nil lock deps should return out-of-sync error)

- [x] this PR contains unit tests


Patch:
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index 407e7ffe4..4e9d691d8 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -79,7 +79,12 @@ func (m *Manager) Build() error {
 		return m.Update()
 	}
 
+	// Check that all of the repos we're dependent on actually exist.
 	req := c.Metadata.Dependencies
+	if _, err := m.resolveRepoNames(req); err != nil {
+		return err
+	}
+
 	if sum, err := resolver.HashReq(req, lock.Dependencies); err != nil || sum != lock.Digest {
 		return errors.New("Chart.lock is out of sync with Chart.yaml")
 	}
@@ -120,7 +125,7 @@ func (m *Manager) Update() error {
 
 	// Check that all of the repos we're dependent on actually exist and
 	// the repo index names.
-	repoNames, err := m.getRepoNames(req)
+	repoNames, err := m.resolveRepoNames(req)
 	if err != nil {
 		return err
 	}
@@ -372,8 +377,9 @@ Loop:
 	return nil
 }
 
-// getRepoNames returns the repo names of the referenced deps which can be used to fetch the cahced index file.
-func (m *Manager) getRepoNames(deps []*chart.Dependency) (map[string]string, error) {
+// resolveRepoNames returns the repo names of the referenced deps which can be used to fetch the cached index file
+// and replaces aliased repository URLs into resolved URLs in dependencies.
+func (m *Manager) resolveRepoNames(deps []*chart.Dependency) (map[string]string, error) {
 	rf, err := loadRepoConfig(m.RepositoryConfig)
 	if err != nil {
 		if os.IsNotExist(err) {
diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go
index 03adbd3cb..eaadbfb07 100644
--- a/pkg/strvals/parser.go
+++ b/pkg/strvals/parser.go
@@ -84,7 +84,7 @@ func ParseFile(s string, reader RunesValueReader) (map[string]interface{}, error
 	return vals, err
 }
 
-// ParseIntoString parses a strvals line and merges the result into dest.
+// ParseIntoString parses a strvals line nad merges the result into dest.
 //
 // This method always returns a string as the value.
 func ParseIntoString(s string, dest map[string]interface{}) error {
@@ -108,9 +108,6 @@ type RunesValueReader func([]rune) (interface{}, error)
 
 // parser is a simple parser that takes a strvals line and parses it into a
 // map representation.
-//
-// where sc is the source of the original data being parsed
-// where data is the final parsed data from the parses with correct types
 type parser struct {
 	sc     *bytes.Buffer
 	data   map[string]interface{}
@@ -288,13 +285,7 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) {
 		// We have a nested object. Send to t.key
 		inner := map[string]interface{}{}
 		if len(list) > i {
-			var ok bool
-			inner, ok = list[i].(map[string]interface{})
-			if !ok {
-				// We have indices out of order. Initialize empty value.
-				list[i] = map[string]interface{}{}
-				inner = list[i].(map[string]interface{})
-			}
+			inner = list[i].(map[string]interface{})
 		}
 
 		// Recurse
@@ -376,11 +367,6 @@ func inMap(k rune, m map[rune]bool) bool {
 
 func typedVal(v []rune, st bool) interface{} {
 	val := string(v)
-
-	if st {
-		return val
-	}
-
 	if strings.EqualFold(val, "true") {
 		return true
 	}
@@ -389,16 +375,8 @@ func typedVal(v []rune, st bool) interface{} {
 		return false
 	}
 
-	if strings.EqualFold(val, "null") {
-		return nil
-	}
-
-	if strings.EqualFold(val, "0") {
-		return int64(0)
-	}
-
-	// If this value does not start with zero, try parsing it to an int
-	if len(val) != 0 && val[0] != '0' {
+	// If this value does not start with zero, and not returnString, try parsing it to an int
+	if !st && len(val) != 0 && val[0] != '0' {
 		if iv, err := strconv.ParseInt(val, 10, 64); err == nil {
 			return iv
 		}


Test patch:
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index 0c5c08615..6e32b3e85 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -161,7 +161,7 @@ func TestGetRepoNames(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		l, err := m.getRepoNames(tt.req)
+		l, err := m.resolveRepoNames(tt.req)
 		if err != nil {
 			if tt.err {
 				continue
@@ -181,7 +181,8 @@ func TestGetRepoNames(t *testing.T) {
 	}
 }
 
-// This function is the skeleton test code of failing tests for #6416 and bugs due to #5874.
+// This function is the skeleton test code of failing tests for #6416 and #6871 and bugs due to #5874.
+//
 // This function is used by below tests that ensures success of build operation
 // with optional fields, alias, condition, tags, and even with ranged version.
 // Parent chart includes local-subchart 0.1.0 subchart from a fake repository, by default.
@@ -283,3 +284,11 @@ func TestBuild_WithTags(t *testing.T) {
 		Tags: []string{"tag1", "tag2"},
 	})
 }
+
+// Failing test for #6871
+func TestBuild_WithRepositoryAlias(t *testing.T) {
+	// Dependency repository is aliased in Chart.yaml
+	checkBuildWithOptionalFields(t, "with-repository-alias", chart.Dependency{
+		Repository: "@test",
+	})
+}
diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go
index 7f38efa52..ff8d58587 100644
--- a/pkg/strvals/parser_test.go
+++ b/pkg/strvals/parser_test.go
@@ -75,32 +75,12 @@ func TestParseSet(t *testing.T) {
 			expect: map[string]interface{}{"long_int_string": "1234567890"},
 			err:    false,
 		},
-		{
-			str:    "boolean=true",
-			expect: map[string]interface{}{"boolean": "true"},
-			err:    false,
-		},
-		{
-			str:    "is_null=null",
-			expect: map[string]interface{}{"is_null": "null"},
-			err:    false,
-		},
-		{
-			str:    "zero=0",
-			expect: map[string]interface{}{"zero": "0"},
-			err:    false,
-		},
 	}
 	tests := []struct {
 		str    string
 		expect map[string]interface{}
 		err    bool
 	}{
-		{
-			"name1=null,f=false,t=true",
-			map[string]interface{}{"name1": nil, "f": false, "t": true},
-			false,
-		},
 		{
 			"name1=value1",
 			map[string]interface{}{"name1": "value1"},
@@ -128,23 +108,10 @@ func TestParseSet(t *testing.T) {
 			str:    "leading_zeros=00009",
 			expect: map[string]interface{}{"leading_zeros": "00009"},
 		},
-		{
-			str:    "zero_int=0",
-			expect: map[string]interface{}{"zero_int": 0},
-		},
 		{
 			str:    "long_int=1234567890",
 			expect: map[string]interface{}{"long_int": 1234567890},
 		},
-		{
-			str:    "boolean=true",
-			expect: map[string]interface{}{"boolean": true},
-		},
-		{
-			str:    "is_null=null",
-			expect: map[string]interface{}{"is_null": nil},
-			err:    false,
-		},
 		{
 			str: "name1,name2=",
 			err: true,
@@ -303,30 +270,6 @@ func TestParseSet(t *testing.T) {
 			str:    "nested[1][1]=1",
 			expect: map[string]interface{}{"nested": []interface{}{nil, []interface{}{nil, 1}}},
 		},
-		{
-			str: "name1.name2[0].foo=bar,name1.name2[1].foo=bar",
-			expect: map[string]interface{}{
-				"name1": map[string]interface{}{
-					"name2": []map[string]interface{}{{"foo": "bar"}, {"foo": "bar"}},
-				},
-			},
-		},
-		{
-			str: "name1.name2[1].foo=bar,name1.name2[0].foo=bar",
-			expect: map[string]interface{}{
-				"name1": map[string]interface{}{
-					"name2": []map[string]interface{}{{"foo": "bar"}, {"foo": "bar"}},
-				},
-			},
-		},
-		{
-			str: "name1.name2[1].foo=bar",
-			expect: map[string]interface{}{
-				"name1": map[string]interface{}{
-					"name2": []map[string]interface{}{nil, {"foo": "bar"}},
-				},
-			},
-		},
 	}
 
 	for _, tt := range tests {
@@ -388,13 +331,12 @@ func TestParseInto(t *testing.T) {
 			"inner2": "value2",
 		},
 	}
-	input := "outer.inner1=value1,outer.inner3=value3,outer.inner4=4"
+	input := "outer.inner1=value1,outer.inner3=value3"
 	expect := map[string]interface{}{
 		"outer": map[string]interface{}{
 			"inner1": "value1",
 			"inner2": "value2",
 			"inner3": "value3",
-			"inner4": 4,
 		},
 	}
 
@@ -415,39 +357,6 @@ func TestParseInto(t *testing.T) {
 		t.Errorf("%s: Expected:\n%s\nGot:\n%s", input, y1, y2)
 	}
 }
-func TestParseIntoString(t *testing.T) {
-	got := map[string]interface{}{
-		"outer": map[string]interface{}{
-			"inner1": "overwrite",
-			"inner2": "value2",
-		},
-	}
-	input := "outer.inner1=1,outer.inner3=3"
-	expect := map[string]interface{}{
-		"outer": map[string]interface{}{
-			"inner1": "1",
-			"inner2": "value2",
-			"inner3": "3",
-		},
-	}
-
-	if err := ParseIntoString(input, got); err != nil {
-		t.Fatal(err)
-	}
-
-	y1, err := yaml.Marshal(expect)
-	if err != nil {
-		t.Fatal(err)
-	}
-	y2, err := yaml.Marshal(got)
-	if err != nil {
-		t.Fatalf("Error serializing parsed value: %s", err)
-	}
-
-	if string(y1) != string(y2) {
-		t.Errorf("%s: Expected:\n%s\nGot:\n%s", input, y1, y2)
-	}
-}
 
 func TestParseIntoFile(t *testing.T) {
 	got := map[string]interface{}{}
@@ -458,7 +367,7 @@ func TestParseIntoFile(t *testing.T) {
 	rs2v := func(rs []rune) (interface{}, error) {
 		v := string(rs)
 		if v != "path1" {
-			t.Errorf("%s: runesToVal: Expected value path1, got %s", input, v)
+			t.Errorf("%s: RunesValueReader: Expected value path1, got %s", input, v)
 			return "", nil
 		}
 		return "value1", nil