Merge pull request #6070 from bacongobbler/fix-6059

Use chart version as default tag when saving

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)
 		},
 	}
 }

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(&registry.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{

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,15 @@ func NewChartSave(cfg *Configuration) *ChartSave {
 }
 
 // Run executes the chart save operation
-func (a *ChartSave) Run(out io.Writer, path, ref string) error {
+func (a *ChartSave) Run(out io.Writer, ch *chart.Chart, ref string) error {
-	path, err := filepath.Abs(path)
+	r, err := registry.ParseReference(ref)
-	if err != nil {
-		return err
-	}
-
-	ch, err := loader.LoadDir(path)
 	if err != nil {
 		return err
 	}
 
-	r, err := registry.ParseReference(ref)
+	// If no tag is present, use the chart version
-	if err != nil {
+	if r.Tag == "" {
-		return err
+		r.Tag = ch.Metadata.Version
 	}
 
 	return a.cfg.RegistryClient.SaveChart(ch, r)

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)
+	}
+}

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", ref.Tag))
-	tagDir := mkdir(filepath.Join(cache.rootDir, "refs", escape(ref.Repo), "tags", 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 {

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 {

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"
 

pkg/registry/reference_test.go

@@ -22,7 +22,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestReference(t *testing.T) {
+func TestParseReference(t *testing.T) {
 	is := assert.New(t)
 
 	// bad refs