From b3fd254991f394c6d01e97d06fc415b5d2fbfc2e Mon Sep 17 00:00:00 2001 From: Josh Dolitsky Date: Mon, 22 Jul 2019 10:24:52 -0500 Subject: [PATCH 1/2] Use chart version as default tag when saving Signed-off-by: Josh Dolitsky --- pkg/action/chart_save.go | 3 +-- pkg/registry/cache.go | 15 +++------------ pkg/registry/client.go | 12 ------------ pkg/registry/constants.go | 3 --- pkg/registry/reference.go | 18 ++++++++++++++++++ pkg/registry/reference_test.go | 29 ++++++++++++++++++++++++++++- 6 files changed, 50 insertions(+), 30 deletions(-) diff --git a/pkg/action/chart_save.go b/pkg/action/chart_save.go index 88ed25b36..26a633725 100644 --- a/pkg/action/chart_save.go +++ b/pkg/action/chart_save.go @@ -48,10 +48,9 @@ func (a *ChartSave) Run(out io.Writer, path, ref string) error { return err } - r, err := registry.ParseReference(ref) + r, err := registry.ParseReferenceWithChartDefaults(ref, ch) if err != nil { return err } - return a.cfg.RegistryClient.SaveChart(ch, r) } diff --git a/pkg/registry/cache.go b/pkg/registry/cache.go index 151b13811..feb9e8069 100644 --- a/pkg/registry/cache.go +++ b/pkg/registry/cache.go @@ -142,7 +142,7 @@ func (cache *filesystemCache) ChartToLayers(ch *chart.Chart) ([]ocispec.Descript } func (cache *filesystemCache) LoadReference(ref *Reference) ([]ocispec.Descriptor, error) { - tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tagOrDefault(ref.Tag)) + tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", ref.Tag) // add meta layer metaJSONRaw, err := getSymlinkDestContent(filepath.Join(tagDir, "meta")) @@ -170,8 +170,7 @@ func (cache *filesystemCache) LoadReference(ref *Reference) ([]ocispec.Descripto } func (cache *filesystemCache) StoreReference(ref *Reference, layers []ocispec.Descriptor) (bool, error) { - tag := tagOrDefault(ref.Tag) - tagDir := mkdir(filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tag)) + tagDir := mkdir(filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", ref.Tag)) // Retrieve just the meta and content layers metaLayer, contentLayer, err := extractLayers(layers) @@ -244,7 +243,7 @@ func (cache *filesystemCache) StoreReference(ref *Reference, layers []ocispec.De } func (cache *filesystemCache) DeleteReference(ref *Reference) error { - tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", tagOrDefault(ref.Tag)) + tagDir := filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", ref.Tag) if _, err := os.Stat(tagDir); os.IsNotExist(err) { return errors.New("ref not found") } @@ -401,14 +400,6 @@ func byteCountBinary(b int64) string { return fmt.Sprintf("%.1f %ciB", float64(b)/float64(div), "KMGTPE"[exp]) } -// tagOrDefault returns the tag if present, if not the default tag -func tagOrDefault(tag string) string { - if tag != "" { - return tag - } - return HelmChartDefaultTag -} - // shortDigest returns first 7 characters of a sha256 digest func shortDigest(digest string) string { if len(digest) == 64 { diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 93ef04023..844db562d 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -91,7 +91,6 @@ func (c *Client) Logout(hostname string) error { // PushChart uploads a chart to a registry func (c *Client) PushChart(ref *Reference) error { - c.setDefaultTag(ref) fmt.Fprintf(c.out, "The push refers to repository [%s]\n", ref.Repo) layers, err := c.cache.LoadReference(ref) if err != nil { @@ -113,7 +112,6 @@ func (c *Client) PushChart(ref *Reference) error { // PullChart downloads a chart from a registry func (c *Client) PullChart(ref *Reference) error { - c.setDefaultTag(ref) fmt.Fprintf(c.out, "%s: Pulling from %s\n", ref.Tag, ref.Repo) _, layers, err := oras.Pull(c.newContext(), c.resolver, ref.String(), c.cache.store, oras.WithAllowedMediaTypes(KnownMediaTypes())) if err != nil { @@ -133,7 +131,6 @@ func (c *Client) PullChart(ref *Reference) error { // SaveChart stores a copy of chart in local cache func (c *Client) SaveChart(ch *chart.Chart, ref *Reference) error { - c.setDefaultTag(ref) layers, err := c.cache.ChartToLayers(ch) if err != nil { return err @@ -148,7 +145,6 @@ func (c *Client) SaveChart(ch *chart.Chart, ref *Reference) error { // LoadChart retrieves a chart object by reference func (c *Client) LoadChart(ref *Reference) (*chart.Chart, error) { - c.setDefaultTag(ref) layers, err := c.cache.LoadReference(ref) if err != nil { return nil, err @@ -159,7 +155,6 @@ func (c *Client) LoadChart(ref *Reference) (*chart.Chart, error) { // RemoveChart deletes a locally saved chart func (c *Client) RemoveChart(ref *Reference) error { - c.setDefaultTag(ref) err := c.cache.DeleteReference(ref) if err != nil { return err @@ -184,13 +179,6 @@ func (c *Client) PrintChartTable() error { return nil } -func (c *Client) setDefaultTag(ref *Reference) { - if ref.Tag == "" { - ref.Tag = HelmChartDefaultTag - fmt.Fprintf(c.out, "Using default tag: %s\n", HelmChartDefaultTag) - } -} - // disable verbose logging coming from ORAS unless debug is enabled func (c *Client) newContext() context.Context { if !c.debug { diff --git a/pkg/registry/constants.go b/pkg/registry/constants.go index 76458b38d..6dc46f2c1 100644 --- a/pkg/registry/constants.go +++ b/pkg/registry/constants.go @@ -17,9 +17,6 @@ limitations under the License. package registry // import "helm.sh/helm/pkg/registry" const ( - // HelmChartDefaultTag is the default tag used when storing a chart reference with no tag - HelmChartDefaultTag = "latest" - // HelmChartConfigMediaType is the reserved media type for the Helm chart manifest config HelmChartConfigMediaType = "application/vnd.cncf.helm.config.v1+json" diff --git a/pkg/registry/reference.go b/pkg/registry/reference.go index 7a136205f..b1887f054 100644 --- a/pkg/registry/reference.go +++ b/pkg/registry/reference.go @@ -22,6 +22,8 @@ import ( "strings" "github.com/containerd/containerd/reference" + + "helm.sh/helm/pkg/chart" ) var ( @@ -59,6 +61,22 @@ func ParseReference(s string) (*Reference, error) { return &ref, nil } +// ParseReferenceWithChartDefaults converts a string to a Reference, +// using values from a given chart as defaults +func ParseReferenceWithChartDefaults(s string, ch *chart.Chart) (*Reference, error) { + ref, err := ParseReference(s) + if err != nil { + return nil, err + } + + // If no tag is present, use the chart version + if ref.Tag == "" { + ref.Tag = ch.Metadata.Version + } + + return ref, nil +} + // setExtraFields adds the Repo and Tag fields to a Reference func (ref *Reference) setExtraFields() { ref.Tag = ref.Object diff --git a/pkg/registry/reference_test.go b/pkg/registry/reference_test.go index e9ec024bc..4bdb22c5d 100644 --- a/pkg/registry/reference_test.go +++ b/pkg/registry/reference_test.go @@ -20,9 +20,11 @@ import ( "testing" "github.com/stretchr/testify/assert" + + "helm.sh/helm/pkg/chart" ) -func TestReference(t *testing.T) { +func TestParseReference(t *testing.T) { is := assert.New(t) // bad refs @@ -87,3 +89,28 @@ func TestReference(t *testing.T) { is.Equal("my.host.com/my/nested/repo", ref.Repo) is.Equal("1.2.3", ref.Tag) } + +func TestParseReferenceWithChartDefaults(t *testing.T) { + is := assert.New(t) + + ch := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "mychart", + Version: "1.5.0", + }, + } + + // If tag provided, use tag (1.2.3) + s := "my.host.com/my/nested/repo:1.2.3" + ref, err := ParseReferenceWithChartDefaults(s, ch) + is.NoError(err) + is.Equal("my.host.com/my/nested/repo", ref.Repo) + is.Equal("1.2.3", ref.Tag) + + // If tag NOT provided, use version from chart (1.5.0) + s = "my.host.com/my/nested/repo" + ref, err = ParseReferenceWithChartDefaults(s, ch) + is.NoError(err) + is.Equal("my.host.com/my/nested/repo", ref.Repo) + is.Equal("1.5.0", ref.Tag) +} From 2045fab01fa33554d15b386a55776f57d3efab9a Mon Sep 17 00:00:00 2001 From: Matthew Fisher Date: Tue, 23 Jul 2019 14:33:23 -0700 Subject: [PATCH 2/2] ref(action): remove ParseReferenceWithChartDefaults Signed-off-by: Matthew Fisher --- cmd/helm/chart_save.go | 15 +++++++- pkg/action/action_test.go | 32 ++++++++++++++++- pkg/action/chart_save.go | 17 ++++----- pkg/action/chart_save_test.go | 63 ++++++++++++++++++++++++++++++++++ pkg/registry/reference.go | 18 ---------- pkg/registry/reference_test.go | 27 --------------- 6 files changed, 114 insertions(+), 58 deletions(-) create mode 100644 pkg/action/chart_save_test.go diff --git a/cmd/helm/chart_save.go b/cmd/helm/chart_save.go index c04bde9ba..1f8c915bb 100644 --- a/cmd/helm/chart_save.go +++ b/cmd/helm/chart_save.go @@ -18,11 +18,13 @@ package main import ( "io" + "path/filepath" "github.com/spf13/cobra" "helm.sh/helm/cmd/helm/require" "helm.sh/helm/pkg/action" + "helm.sh/helm/pkg/chart/loader" ) const chartSaveDesc = ` @@ -41,7 +43,18 @@ func newChartSaveCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { path := args[0] ref := args[1] - return action.NewChartSave(cfg).Run(out, path, ref) + + path, err := filepath.Abs(path) + if err != nil { + return err + } + + ch, err := loader.LoadDir(path) + if err != nil { + return err + } + + return action.NewChartSave(cfg).Run(out, ch, ref) }, } } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index cb966dcb8..83346ea58 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -16,16 +16,19 @@ limitations under the License. package action import ( + "context" "flag" "io/ioutil" "testing" "time" + dockerauth "github.com/deislabs/oras/pkg/auth/docker" fakeclientset "k8s.io/client-go/kubernetes/fake" "helm.sh/helm/pkg/chart" "helm.sh/helm/pkg/chartutil" kubefake "helm.sh/helm/pkg/kube/fake" + "helm.sh/helm/pkg/registry" "helm.sh/helm/pkg/release" "helm.sh/helm/pkg/storage" "helm.sh/helm/pkg/storage/driver" @@ -36,10 +39,35 @@ var verbose = flag.Bool("test.log", false, "enable test logging") func actionConfigFixture(t *testing.T) *Configuration { t.Helper() + client, err := dockerauth.NewClient() + if err != nil { + t.Fatal(err) + } + + resolver, err := client.Resolver(context.Background()) + if err != nil { + t.Fatal(err) + } + + tdir, err := ioutil.TempDir("", "helm-action-test") + if err != nil { + t.Fatal(err) + } + return &Configuration{ Releases: storage.Init(driver.NewMemory()), KubeClient: &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}}, Capabilities: chartutil.DefaultCapabilities, + RegistryClient: registry.NewClient(®istry.ClientOptions{ + Out: ioutil.Discard, + Authorizer: registry.Authorizer{ + Client: client, + }, + Resolver: registry.Resolver{ + Resolver: resolver, + }, + CacheRootDir: tdir, + }), Log: func(format string, v ...interface{}) { t.Helper() if *verbose { @@ -106,7 +134,9 @@ func buildChart(opts ...chartOption) *chart.Chart { Chart: &chart.Chart{ // TODO: This should be more complete. Metadata: &chart.Metadata{ - Name: "hello", + APIVersion: "v1", + Name: "hello", + Version: "0.1.0", }, // This adds a basic template and hooks. Templates: []*chart.File{ diff --git a/pkg/action/chart_save.go b/pkg/action/chart_save.go index 26a633725..3f8dcf533 100644 --- a/pkg/action/chart_save.go +++ b/pkg/action/chart_save.go @@ -18,9 +18,8 @@ package action import ( "io" - "path/filepath" - "helm.sh/helm/pkg/chart/loader" + "helm.sh/helm/pkg/chart" "helm.sh/helm/pkg/registry" ) @@ -37,20 +36,16 @@ func NewChartSave(cfg *Configuration) *ChartSave { } // Run executes the chart save operation -func (a *ChartSave) Run(out io.Writer, path, ref string) error { - path, err := filepath.Abs(path) +func (a *ChartSave) Run(out io.Writer, ch *chart.Chart, ref string) error { + r, err := registry.ParseReference(ref) if err != nil { return err } - ch, err := loader.LoadDir(path) - if err != nil { - return err + // If no tag is present, use the chart version + if r.Tag == "" { + r.Tag = ch.Metadata.Version } - r, err := registry.ParseReferenceWithChartDefaults(ref, ch) - if err != nil { - return err - } return a.cfg.RegistryClient.SaveChart(ch, r) } diff --git a/pkg/action/chart_save_test.go b/pkg/action/chart_save_test.go new file mode 100644 index 000000000..609fc79ed --- /dev/null +++ b/pkg/action/chart_save_test.go @@ -0,0 +1,63 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package action + +import ( + "io/ioutil" + "testing" + + "helm.sh/helm/pkg/registry" +) + +func chartSaveAction(t *testing.T) *ChartSave { + t.Helper() + config := actionConfigFixture(t) + action := NewChartSave(config) + return action +} + +func TestChartSave(t *testing.T) { + action := chartSaveAction(t) + + input := buildChart() + if err := action.Run(ioutil.Discard, input, "localhost:5000/test:0.2.0"); err != nil { + t.Error(err) + } + + ref, err := registry.ParseReference("localhost:5000/test:0.2.0") + if err != nil { + t.Fatal(err) + } + + if _, err := action.cfg.RegistryClient.LoadChart(ref); err != nil { + t.Error(err) + } + + // now let's check if `helm chart save` can use the chart version when the tag is not present + if err := action.Run(ioutil.Discard, input, "localhost:5000/test"); err != nil { + t.Error(err) + } + + ref, err = registry.ParseReference("localhost:5000/test:0.1.0") + if err != nil { + t.Fatal(err) + } + + if _, err := action.cfg.RegistryClient.LoadChart(ref); err != nil { + t.Error(err) + } +} diff --git a/pkg/registry/reference.go b/pkg/registry/reference.go index b1887f054..7a136205f 100644 --- a/pkg/registry/reference.go +++ b/pkg/registry/reference.go @@ -22,8 +22,6 @@ import ( "strings" "github.com/containerd/containerd/reference" - - "helm.sh/helm/pkg/chart" ) var ( @@ -61,22 +59,6 @@ func ParseReference(s string) (*Reference, error) { return &ref, nil } -// ParseReferenceWithChartDefaults converts a string to a Reference, -// using values from a given chart as defaults -func ParseReferenceWithChartDefaults(s string, ch *chart.Chart) (*Reference, error) { - ref, err := ParseReference(s) - if err != nil { - return nil, err - } - - // If no tag is present, use the chart version - if ref.Tag == "" { - ref.Tag = ch.Metadata.Version - } - - return ref, nil -} - // setExtraFields adds the Repo and Tag fields to a Reference func (ref *Reference) setExtraFields() { ref.Tag = ref.Object diff --git a/pkg/registry/reference_test.go b/pkg/registry/reference_test.go index 4bdb22c5d..7f0299341 100644 --- a/pkg/registry/reference_test.go +++ b/pkg/registry/reference_test.go @@ -20,8 +20,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - - "helm.sh/helm/pkg/chart" ) func TestParseReference(t *testing.T) { @@ -89,28 +87,3 @@ func TestParseReference(t *testing.T) { is.Equal("my.host.com/my/nested/repo", ref.Repo) is.Equal("1.2.3", ref.Tag) } - -func TestParseReferenceWithChartDefaults(t *testing.T) { - is := assert.New(t) - - ch := &chart.Chart{ - Metadata: &chart.Metadata{ - Name: "mychart", - Version: "1.5.0", - }, - } - - // If tag provided, use tag (1.2.3) - s := "my.host.com/my/nested/repo:1.2.3" - ref, err := ParseReferenceWithChartDefaults(s, ch) - is.NoError(err) - is.Equal("my.host.com/my/nested/repo", ref.Repo) - is.Equal("1.2.3", ref.Tag) - - // If tag NOT provided, use version from chart (1.5.0) - s = "my.host.com/my/nested/repo" - ref, err = ParseReferenceWithChartDefaults(s, ch) - is.NoError(err) - is.Equal("my.host.com/my/nested/repo", ref.Repo) - is.Equal("1.5.0", ref.Tag) -}