From 7061716406e60294922a133c8b7b4631c7ac1e51 Mon Sep 17 00:00:00 2001
From: Matt Butcher <matt.butcher@microsoft.com>
Date: Wed, 31 Oct 2018 16:15:08 -0600
Subject: [PATCH] ref: require name by default on 'helm install' (#4858)
This is described in the official Helm 3 proposal: https://github.com/helm/community/blob/master/helm-v3/000-helm-v3.md
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
---
Gopkg.lock | 9 ---
Gopkg.toml | 4 --
Makefile | 5 +-
cmd/helm/install.go | 72 ++++++++++++++++----
cmd/helm/install_test.go | 30 ++++----
cmd/helm/template.go | 2 +-
cmd/helm/testdata/output/install-no-args.txt | 4 +-
pkg/tiller/release_install_test.go | 35 +++++++---
pkg/tiller/release_server.go | 59 ++++++----------
pkg/tiller/release_server_test.go | 2 +-
pkg/tiller/release_update_test.go | 1 +
11 files changed, 129 insertions(+), 94 deletions(-)
diff --git a/Gopkg.lock b/Gopkg.lock
index cd5e78f7f..7bef996b8 100644
--- a/Gopkg.lock
+++ b/Gopkg.lock
@@ -561,14 +561,6 @@
pruneopts = "UT"
revision = "e3a8ff8ce36581f87a15341206f205b1da467059"
-[[projects]]
- branch = "master"
- digest = "1:9123998e9b4a6ed0fcf9cae137a6cd9e265a5d18823e34d1cd12e9d9845b2719"
- name = "github.com/technosophos/moniker"
- packages = ["."]
- pruneopts = "UT"
- revision = "ab470f5e105a44d0c87ea21bacd6a335c4816d83"
-
[[projects]]
digest = "1:9601e4354239b69f62c86d24c74a19d7c7e3c7f7d2d9f01d42e5830b4673e121"
name = "golang.org/x/crypto"
@@ -1174,7 +1166,6 @@
"github.com/spf13/cobra/doc",
"github.com/spf13/pflag",
"github.com/stretchr/testify/assert",
- "github.com/technosophos/moniker",
"golang.org/x/crypto/openpgp",
"golang.org/x/crypto/openpgp/clearsign",
"golang.org/x/crypto/openpgp/errors",
diff --git a/Gopkg.toml b/Gopkg.toml
index 3e558b9ba..e1f48dc40 100644
--- a/Gopkg.toml
+++ b/Gopkg.toml
@@ -27,10 +27,6 @@
name = "github.com/gosuri/uitable"
branch = "master"
-[[constraint]]
- name = "github.com/technosophos/moniker"
- branch = "master"
-
[[constraint]]
name = "k8s.io/api"
branch = "release-1.12"
diff --git a/Makefile b/Makefile
index 394a4a2e0..1fbc5ecf4 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,7 @@
BINDIR := $(CURDIR)/bin
DIST_DIRS := find * -type d -exec
TARGETS := darwin/amd64 linux/amd64 linux/386 linux/arm linux/arm64 linux/ppc64le windows/amd64
+BINNAME ?= helm
# go option
GO ?= go
@@ -41,12 +42,12 @@ all: build
.PHONY: build
build:
- $(GO) build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o $(BINDIR)/helm k8s.io/helm/cmd/helm
+ $(GO) build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o $(BINDIR)/$(BINNAME) k8s.io/helm/cmd/helm
.PHONY: build-cross
build-cross: LDFLAGS += -extldflags "-static"
build-cross:
- CGO_ENABLED=0 gox -parallel=3 -output="_dist/{{.OS}}-{{.Arch}}/{{.Dir}}" -osarch='$(TARGETS)' $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' k8s.io/helm/cmd/helm
+ CGO_ENABLED=0 gox -parallel=3 -output="_dist/{{.OS}}-{{.Arch}}/$(BINNAME)" -osarch='$(TARGETS)' $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' k8s.io/helm/cmd/helm
.PHONY: dist
dist:
diff --git a/cmd/helm/install.go b/cmd/helm/install.go
index 8178f023c..09b1bec51 100644
--- a/cmd/helm/install.go
+++ b/cmd/helm/install.go
@@ -20,8 +20,10 @@ import (
"bytes"
"fmt"
"io"
+ "path/filepath"
"strings"
"text/template"
+ "time"
"github.com/Masterminds/sprig"
"github.com/pkg/errors"
@@ -46,27 +48,27 @@ To override values in a chart, use either the '--values' flag and pass in a file
or use the '--set' flag and pass configuration from the command line, to force
a string value use '--set-string'.
- $ helm install -f myvalues.yaml ./redis
+ $ helm install -f myvalues.yaml myredis ./redis
or
- $ helm install --set name=prod ./redis
+ $ helm install --set name=prod myredis ./redis
or
- $ helm install --set-string long_int=1234567890 ./redis
+ $ helm install --set-string long_int=1234567890 myredis ./redis
You can specify the '--values'/'-f' flag multiple times. The priority will be given to the
last (right-most) file specified. For example, if both myvalues.yaml and override.yaml
contained a key called 'Test', the value set in override.yaml would take precedence:
- $ helm install -f myvalues.yaml -f override.yaml ./redis
+ $ helm install -f myvalues.yaml -f override.yaml myredis ./redis
You can specify the '--set' flag multiple times. The priority will be given to the
last (right-most) set specified. For example, if both 'bar' and 'newbar' values are
set for a key called 'foo', the 'newbar' value would take precedence:
- $ helm install --set foo=bar --set foo=newbar ./redis
+ $ helm install --set foo=bar --set foo=newbar myredis ./redis
To check the generated manifests of a release without installing the chart,
@@ -99,7 +101,7 @@ charts in a repository, use 'helm search'.
`
type installOptions struct {
- name string // --name
+ name string // arg 0
dryRun bool // --dry-run
disableHooks bool // --disable-hooks
replace bool // --replace
@@ -108,7 +110,8 @@ type installOptions struct {
wait bool // --wait
devel bool // --devel
depUp bool // --dep-up
- chartPath string // arg
+ chartPath string // arg 1
+ generateName bool // --generate-name
valuesOptions
chartPathOptions
@@ -120,10 +123,10 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
o := &installOptions{client: c}
cmd := &cobra.Command{
- Use: "install [CHART]",
- Short: "install a chart archive",
+ Use: "install [NAME] [CHART]",
+ Short: "install a chart",
Long: installDesc,
- Args: require.ExactArgs(1),
+ Args: require.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
debug("Original chart version: %q", o.version)
if o.version == "" && o.devel {
@@ -131,7 +134,13 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
o.version = ">0.0.0-0"
}
- cp, err := o.locateChart(args[0])
+ name, chart, err := o.nameAndChart(args)
+ if err != nil {
+ return err
+ }
+ o.name = name // FIXME
+
+ cp, err := o.locateChart(chart)
if err != nil {
return err
}
@@ -142,7 +151,7 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
}
f := cmd.Flags()
- f.StringVarP(&o.name, "name", "", "", "release name. If unspecified, it will autogenerate one for you")
+ f.BoolVarP(&o.generateName, "generate-name", "g", false, "generate the name (and omit the NAME parameter)")
f.BoolVar(&o.dryRun, "dry-run", false, "simulate an install")
f.BoolVar(&o.disableHooks, "no-hooks", false, "prevent hooks from running during install")
f.BoolVar(&o.replace, "replace", false, "re-use the given name, even if that name is already used. This is unsafe in production")
@@ -157,6 +166,41 @@ func newInstallCmd(c helm.Interface, out io.Writer) *cobra.Command {
return cmd
}
+// nameAndChart returns the name of the release and the chart that should be used.
+//
+// This will read the flags and handle name generation if necessary.
+func (o *installOptions) nameAndChart(args []string) (string, string, error) {
+ flagsNotSet := func() error {
+ if o.generateName {
+ return errors.New("cannot set --generate-name and also specify a name")
+ }
+ if o.nameTemplate != "" {
+ return errors.New("cannot set --name-template and also specify a name")
+ }
+ return nil
+ }
+ if len(args) == 2 {
+ return args[0], args[1], flagsNotSet()
+ }
+
+ if o.nameTemplate != "" {
+ newName, err := templateName(o.nameTemplate)
+ return newName, args[0], err
+ }
+
+ if !o.generateName {
+ return "", args[0], errors.New("must either provide a name or specify --generate-name")
+ }
+
+ base := filepath.Base(args[0])
+ if base == "." || base == "" {
+ base = "chart"
+ }
+ newName := fmt.Sprintf("%s-%d", base, time.Now().Unix())
+
+ return newName, args[0], nil
+}
+
func (o *installOptions) run(out io.Writer) error {
debug("CHART PATH: %s\n", o.chartPath)
@@ -167,7 +211,7 @@ func (o *installOptions) run(out io.Writer) error {
// If template is specified, try to run the template.
if o.nameTemplate != "" {
- o.name, err = generateName(o.nameTemplate)
+ o.name, err = templateName(o.nameTemplate)
if err != nil {
return err
}
@@ -276,7 +320,7 @@ func (o *installOptions) printRelease(out io.Writer, rel *release.Release) {
}
}
-func generateName(nameTemplate string) (string, error) {
+func templateName(nameTemplate string) (string, error) {
t, err := template.New("name-template").Funcs(sprig.TxtFuncMap()).Parse(nameTemplate)
if err != nil {
return "", err
diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go
index 0a0d0d351..c0d93a15c 100644
--- a/cmd/helm/install_test.go
+++ b/cmd/helm/install_test.go
@@ -27,37 +27,37 @@ func TestInstall(t *testing.T) {
// Install, base case
{
name: "basic install",
- cmd: "install testdata/testcharts/alpine --name aeneas",
+ cmd: "install aeneas testdata/testcharts/alpine ",
golden: "output/install.txt",
},
// Install, no hooks
{
name: "install without hooks",
- cmd: "install testdata/testcharts/alpine --name aeneas --no-hooks",
+ cmd: "install aeneas testdata/testcharts/alpine --no-hooks",
golden: "output/install-no-hooks.txt",
},
// Install, values from cli
{
name: "install with values",
- cmd: "install testdata/testcharts/alpine --name virgil --set foo=bar",
+ cmd: "install virgil testdata/testcharts/alpine --set foo=bar",
golden: "output/install-with-values.txt",
},
// Install, values from cli via multiple --set
{
name: "install with multiple values",
- cmd: "install testdata/testcharts/alpine --name virgil --set foo=bar --set bar=foo",
+ cmd: "install virgil testdata/testcharts/alpine --set foo=bar --set bar=foo",
golden: "output/install-with-multiple-values.txt",
},
// Install, values from yaml
{
name: "install with values file",
- cmd: "install testdata/testcharts/alpine --name virgil -f testdata/testcharts/alpine/extra_values.yaml",
+ cmd: "install virgil testdata/testcharts/alpine -f testdata/testcharts/alpine/extra_values.yaml",
golden: "output/install-with-values-file.txt",
},
// Install, values from multiple yaml
{
name: "install with values",
- cmd: "install testdata/testcharts/alpine --name virgil -f testdata/testcharts/alpine/extra_values.yaml -f testdata/testcharts/alpine/more_values.yaml",
+ cmd: "install virgil testdata/testcharts/alpine -f testdata/testcharts/alpine/extra_values.yaml -f testdata/testcharts/alpine/more_values.yaml",
golden: "output/install-with-multiple-values-files.txt",
},
// Install, no charts
@@ -70,19 +70,19 @@ func TestInstall(t *testing.T) {
// Install, re-use name
{
name: "install and replace release",
- cmd: "install testdata/testcharts/alpine --name aeneas --replace",
+ cmd: "install aeneas testdata/testcharts/alpine --replace",
golden: "output/install-and-replace.txt",
},
// Install, with timeout
{
name: "install with a timeout",
- cmd: "install testdata/testcharts/alpine --name foobar --timeout 120",
+ cmd: "install foobar testdata/testcharts/alpine --timeout 120",
golden: "output/install-with-timeout.txt",
},
// Install, with wait
{
name: "install with a wait",
- cmd: "install testdata/testcharts/alpine --name apollo --wait",
+ cmd: "install apollo testdata/testcharts/alpine --wait",
golden: "output/install-with-wait.txt",
},
// Install, using the name-template
@@ -94,28 +94,28 @@ func TestInstall(t *testing.T) {
// Install, perform chart verification along the way.
{
name: "install with verification, missing provenance",
- cmd: "install testdata/testcharts/compressedchart-0.1.0.tgz --verify --keyring testdata/helm-test-key.pub",
+ cmd: "install bogus testdata/testcharts/compressedchart-0.1.0.tgz --verify --keyring testdata/helm-test-key.pub",
wantError: true,
},
{
name: "install with verification, directory instead of file",
- cmd: "install testdata/testcharts/signtest --verify --keyring testdata/helm-test-key.pub",
+ cmd: "install bogus testdata/testcharts/signtest --verify --keyring testdata/helm-test-key.pub",
wantError: true,
},
{
name: "install with verification, valid",
- cmd: "install testdata/testcharts/signtest-0.1.0.tgz --verify --keyring testdata/helm-test-key.pub",
+ cmd: "install signtest testdata/testcharts/signtest-0.1.0.tgz --verify --keyring testdata/helm-test-key.pub",
},
// Install, chart with missing dependencies in /charts
{
name: "install chart with missing dependencies",
- cmd: "install testdata/testcharts/chart-missing-deps",
+ cmd: "install nodeps testdata/testcharts/chart-missing-deps",
wantError: true,
},
// Install, chart with bad dependencies in Chart.yaml in /charts
{
name: "install chart with bad dependencies in Chart.yaml",
- cmd: "install testdata/testcharts/chart-bad-requirements",
+ cmd: "install badreq testdata/testcharts/chart-bad-requirements",
wantError: true,
},
}
@@ -165,7 +165,7 @@ func TestNameTemplate(t *testing.T) {
for _, tc := range testCases {
- n, err := generateName(tc.tpl)
+ n, err := templateName(tc.tpl)
if err != nil {
if tc.expectedErrorStr == "" {
t.Errorf("Was not expecting error, but got: %v", err)
diff --git a/cmd/helm/template.go b/cmd/helm/template.go
index e94eecd21..4c48f0e6d 100644
--- a/cmd/helm/template.go
+++ b/cmd/helm/template.go
@@ -146,7 +146,7 @@ func (o *templateOptions) run(out io.Writer) error {
// If template is specified, try to run the template.
if o.nameTemplate != "" {
- o.releaseName, err = generateName(o.nameTemplate)
+ o.releaseName, err = templateName(o.nameTemplate)
if err != nil {
return err
}
diff --git a/cmd/helm/testdata/output/install-no-args.txt b/cmd/helm/testdata/output/install-no-args.txt
index faafcb5c2..47f010ab8 100644
--- a/cmd/helm/testdata/output/install-no-args.txt
+++ b/cmd/helm/testdata/output/install-no-args.txt
@@ -1,3 +1,3 @@
-Error: "helm install" requires 1 argument
+Error: "helm install" requires at least 1 argument
-Usage: helm install [CHART] [flags]
+Usage: helm install [NAME] [CHART] [flags]
diff --git a/pkg/tiller/release_install_test.go b/pkg/tiller/release_install_test.go
index 2bcc1207e..47503f93d 100644
--- a/pkg/tiller/release_install_test.go
+++ b/pkg/tiller/release_install_test.go
@@ -28,12 +28,12 @@ import (
func TestInstallRelease(t *testing.T) {
rs := rsFixture(t)
- req := installRequest()
+ req := installRequest(withName("test-install-release"))
res, err := rs.InstallRelease(req)
if err != nil {
t.Fatalf("Failed install: %s", err)
}
- if res.Name == "" {
+ if res.Name != "test-install-release" {
t.Errorf("Expected release name.")
}
if res.Namespace != "spaced" {
@@ -78,11 +78,26 @@ func TestInstallRelease(t *testing.T) {
}
}
+func TestInstallRelease_NoName(t *testing.T) {
+ rs := rsFixture(t)
+
+ // No name supplied here, should cause failure.
+ req := installRequest()
+ _, err := rs.InstallRelease(req)
+ if err == nil {
+ t.Fatal("expected failure when no name is specified")
+ }
+ if !strings.Contains(err.Error(), "name is required") {
+ t.Errorf("Expected message %q to include 'name is required'", err.Error())
+ }
+}
+
func TestInstallRelease_WithNotes(t *testing.T) {
rs := rsFixture(t)
req := installRequest(
withChart(withNotes(notesText)),
+ withName("with-notes"),
)
res, err := rs.InstallRelease(req)
if err != nil {
@@ -141,7 +156,8 @@ func TestInstallRelease_WithNotesRendered(t *testing.T) {
rs := rsFixture(t)
req := installRequest(
- withChart(withNotes(notesText + " {{.Release.Name}}")),
+ withChart(withNotes(notesText+" {{.Release.Name}}")),
+ withName("with-notes"),
)
res, err := rs.InstallRelease(req)
if err != nil {
@@ -203,7 +219,7 @@ func TestInstallRelease_WithChartAndDependencyNotes(t *testing.T) {
req := installRequest(withChart(
withNotes(notesText),
withDependency(withNotes(notesText+" child")),
- ))
+ ), withName("with-chart-and-dependency-notes"))
res, err := rs.InstallRelease(req)
if err != nil {
t.Fatalf("Failed install: %s", err)
@@ -233,13 +249,14 @@ func TestInstallRelease_DryRun(t *testing.T) {
req := installRequest(withDryRun(),
withChart(withSampleTemplates()),
+ withName("test-dry-run"),
)
res, err := rs.InstallRelease(req)
if err != nil {
t.Errorf("Failed install: %s", err)
}
- if res.Name == "" {
- t.Errorf("Expected release name.")
+ if res.Name != "test-dry-run" {
+ t.Errorf("unexpected release name: %q", res.Name)
}
if !strings.Contains(res.Manifest, "---\n# Source: hello/templates/hello\nhello: world") {
@@ -283,7 +300,7 @@ func TestInstallRelease_NoHooks(t *testing.T) {
rs := rsFixture(t)
rs.Releases.Create(releaseStub())
- req := installRequest(withDisabledHooks())
+ req := installRequest(withDisabledHooks(), withName("no-hooks"))
res, err := rs.InstallRelease(req)
if err != nil {
t.Errorf("Failed install: %s", err)
@@ -299,7 +316,7 @@ func TestInstallRelease_FailedHooks(t *testing.T) {
rs.Releases.Create(releaseStub())
rs.KubeClient = newHookFailingKubeClient()
- req := installRequest()
+ req := installRequest(withName("failed-hooks"))
res, err := rs.InstallRelease(req)
if err == nil {
t.Error("Expected failed install")
@@ -345,6 +362,7 @@ func TestInstallRelease_KubeVersion(t *testing.T) {
req := installRequest(
withChart(withKube(">=0.0.0")),
+ withName("kube-version"),
)
_, err := rs.InstallRelease(req)
if err != nil {
@@ -357,6 +375,7 @@ func TestInstallRelease_WrongKubeVersion(t *testing.T) {
req := installRequest(
withChart(withKube(">=5.0.0")),
+ withName("wrong-kube-version"),
)
_, err := rs.InstallRelease(req)
diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go
index 5bd6b501c..5652e5789 100644
--- a/pkg/tiller/release_server.go
+++ b/pkg/tiller/release_server.go
@@ -25,7 +25,6 @@ import (
"time"
"github.com/pkg/errors"
- "github.com/technosophos/moniker"
"gopkg.in/yaml.v2"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/discovery"
@@ -160,47 +159,31 @@ func (s *ReleaseServer) reuseValues(req *hapi.UpdateReleaseRequest, current *rel
func (s *ReleaseServer) uniqName(start string, reuse bool) (string, error) {
- // If a name is supplied, we check to see if that name is taken. If not, it
- // is granted. If reuse is true and a deleted release with that name exists,
- // we re-grant it. Otherwise, an error is returned.
- if start != "" {
-
- if len(start) > releaseNameMaxLen {
- return "", errors.Errorf("release name %q exceeds max length of %d", start, releaseNameMaxLen)
- }
-
- h, err := s.Releases.History(start)
- if err != nil || len(h) < 1 {
- return start, nil
- }
- relutil.Reverse(h, relutil.SortByRevision)
- rel := h[0]
-
- if st := rel.Info.Status; reuse && (st == release.StatusUninstalled || st == release.StatusFailed) {
- // Allowe re-use of names if the previous release is marked deleted.
- s.Log("name %s exists but is not in use, reusing name", start)
- return start, nil
- } else if reuse {
- return "", errors.New("cannot re-use a name that is still in use")
- }
+ if start == "" {
+ return "", errors.New("name is required")
+ }
- return "", errors.Errorf("a release named %s already exists.\nRun: helm ls --all %s; to check the status of the release\nOr run: helm del --purge %s; to delete it", start, start, start)
+ if len(start) > releaseNameMaxLen {
+ return "", errors.Errorf("release name %q exceeds max length of %d", start, releaseNameMaxLen)
}
- maxTries := 5
- for i := 0; i < maxTries; i++ {
- namer := moniker.New()
- name := namer.NameSep("-")
- if len(name) > releaseNameMaxLen {
- name = name[:releaseNameMaxLen]
- }
- if _, err := s.Releases.Get(name, 1); strings.Contains(err.Error(), "not found") {
- return name, nil
- }
- s.Log("info: generated name %s is taken. Searching again.", name)
+ h, err := s.Releases.History(start)
+ if err != nil || len(h) < 1 {
+ return start, nil
+ }
+ relutil.Reverse(h, relutil.SortByRevision)
+ rel := h[0]
+
+ if st := rel.Info.Status; reuse && (st == release.StatusUninstalled || st == release.StatusFailed) {
+ // Allowe re-use of names if the previous release is marked deleted.
+ s.Log("name %s exists but is not in use, reusing name", start)
+ return start, nil
+ } else if reuse {
+ return "", errors.New("cannot re-use a name that is still in use")
}
- s.Log("warning: No available release names found after %d tries", maxTries)
- return "ERROR", errors.New("no available release name found")
+
+ return "", errors.Errorf("a release named %s already exists.\nRun: helm ls --all %s; to check the status of the release\nOr run: helm del --purge %s; to delete it", start, start, start)
+
}
// capabilities builds a Capabilities from discovery information.
diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go
index 3115341b6..cec06a35c 100644
--- a/pkg/tiller/release_server_test.go
+++ b/pkg/tiller/release_server_test.go
@@ -334,8 +334,8 @@ func TestUniqName(t *testing.T) {
reuse bool
err bool
}{
+ {"", "", false, true}, // Blank name is illegal
{"first", "first", false, false},
- {"", "[a-z]+-[a-z]+", false, false},
{"angry-panda", "", false, true},
{"happy-panda", "", false, true},
{"happy-panda", "happy-panda", true, false},
diff --git a/pkg/tiller/release_update_test.go b/pkg/tiller/release_update_test.go
index c3781d1cf..178522d08 100644
--- a/pkg/tiller/release_update_test.go
+++ b/pkg/tiller/release_update_test.go
@@ -130,6 +130,7 @@ func TestUpdateRelease_ComplexReuseValues(t *testing.T) {
rs := rsFixture(t)
installReq := &hapi.InstallReleaseRequest{
+ Name: "complex-reuse-values",
Namespace: "spaced",
Chart: &chart.Chart{
Metadata: &chart.Metadata{Name: "hello"},
--
GitLab