From 17553db485f6165c15aa17c6dfcc7589edb025ee Mon Sep 17 00:00:00 2001 From: Hang Park Date: Wed, 13 Nov 2019 02:33:27 +0900 Subject: [PATCH 1/2] fix(pkg/downloader): add failing test for build with repo alias Signed-off-by: Hang Park --- pkg/downloader/manager_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 0c5c08615..d6cae4e51 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -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", + }) +} From 0987c6f7b91bffc360a8c604b9dfb87b845cc919 Mon Sep 17 00:00:00 2001 From: Hang Park Date: Wed, 13 Nov 2019 02:35:48 +0900 Subject: [PATCH 2/2] fix(pkg/downloader): resolve repo alias before checking digests on build `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 their input data in general. Signed-off-by: Hang Park --- pkg/downloader/manager.go | 12 +++++++++--- pkg/downloader/manager_test.go | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) 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/downloader/manager_test.go b/pkg/downloader/manager_test.go index d6cae4e51..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