From 628a629a675738dc42cbbd0ff22d61bc7928337f Mon Sep 17 00:00:00 2001 From: Yusuke KUOKA Date: Fri, 24 Aug 2018 16:17:32 +0900 Subject: [PATCH] rfix helmfile should fail on duplicate release name after filtered by labels This is a follow-up for #218, fixes the unintentional degradation that broken the use-case described in https://github.com/roboll/helmfile/issues/193#issuecomment-415434408 --- main.go | 33 ++++++++++++++++++++++++--------- main_test.go | 26 ++++++++++++++++++++++++++ state/state.go | 24 +++++++----------------- state/state_test.go | 35 ++++++----------------------------- 4 files changed, 63 insertions(+), 55 deletions(-) create mode 100644 main_test.go diff --git a/main.go b/main.go index faa46af36..884d42515 100644 --- a/main.go +++ b/main.go @@ -397,7 +397,18 @@ func eachDesiredStateDo(c *cli.Context, converge func(*state.HelmState, helmexec } allSelectorNotMatched := true for _, f := range desiredStateFiles { - state, helm, noReleases, err := loadDesiredStateFromFile(c, f) + yamlBuf, err := state.RenderTemplateFileToBuffer(f) + if err != nil { + return err + } + state, helm, noReleases, err := loadDesiredStateFromFile( + yamlBuf.Bytes(), + f, + c.GlobalString("kube-context"), + c.GlobalString("namespace"), + c.GlobalStringSlice("selector"), + c.App.Metadata["logger"].(*zap.SugaredLogger), + ) if err != nil { return err } @@ -474,14 +485,8 @@ func directoryExistsAt(path string) bool { return err == nil && fileInfo.Mode().IsDir() } -func loadDesiredStateFromFile(c *cli.Context, file string) (*state.HelmState, helmexec.Interface, bool, error) { - kubeContext := c.GlobalString("kube-context") - namespace := c.GlobalString("namespace") - labels := c.GlobalStringSlice("selector") - - logger := c.App.Metadata["logger"].(*zap.SugaredLogger) - - st, err := state.CreateFromFile(file, logger) +func loadDesiredStateFromFile(yaml []byte, file string, kubeContext, namespace string, labels []string, logger *zap.SugaredLogger) (*state.HelmState, helmexec.Interface, bool, error) { + st, err := state.CreateFromYaml(yaml, file, logger) if err != nil { return nil, nil, false, fmt.Errorf("failed to read %s: %v", file, err) } @@ -510,6 +515,16 @@ func loadDesiredStateFromFile(c *cli.Context, file string) (*state.HelmState, he } } + releaseNameCounts := map[string]int{} + for _, r := range st.Releases { + releaseNameCounts[r.Name] += 1 + } + for name, c := range releaseNameCounts { + if c > 1 { + return nil, nil, false, fmt.Errorf("duplicate release \"%s\" found: there were %d releases named \"%s\" matching specified selector", name, c, name) + } + } + sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) go func() { diff --git a/main_test.go b/main_test.go new file mode 100644 index 000000000..fe654c404 --- /dev/null +++ b/main_test.go @@ -0,0 +1,26 @@ +package main + +import "testing" + +// See https://github.com/roboll/helmfile/issues/193 +func TestReadFromYaml_DuplicateReleaseName(t *testing.T) { + yamlFile := "example/path/to/yaml/file" + yamlContent := []byte(`releases: +- name: myrelease1 + chart: mychart1 + labels: + stage: pre + foo: bar +- name: myrelease1 + chart: mychart2 + labels: + stage: post +`) + _, _, _, err := loadDesiredStateFromFile(yamlContent, yamlFile, "default", "default", []string{}, logger) + if err == nil { + t.Error("error expected but not happened") + } + if err.Error() != "duplicate release \"myrelease1\" found: there were 2 releases named \"myrelease1\" matching specified selector" { + t.Errorf("unexpected error happened: %v", err) + } +} diff --git a/state/state.go b/state/state.go index 0c719f502..9562236d7 100644 --- a/state/state.go +++ b/state/state.go @@ -84,17 +84,17 @@ type SetValue struct { File string `yaml:"file"` } -// CreateFromFile loads the helmfile from disk and processes the template -func CreateFromFile(file string, logger *zap.SugaredLogger) (*HelmState, error) { - yamlBuf, err := renderTemplateFileToBuffer(file) +// CreateFromTemplateFile loads the helmfile from disk and processes the template +func CreateFromTemplateFile(file string, logger *zap.SugaredLogger) (*HelmState, error) { + yamlBuf, err := RenderTemplateFileToBuffer(file) if err != nil { return nil, err } - return readFromYaml(yamlBuf.Bytes(), file, logger) + return CreateFromYaml(yamlBuf.Bytes(), file, logger) } -func readFromYaml(content []byte, file string, logger *zap.SugaredLogger) (*HelmState, error) { +func CreateFromYaml(content []byte, file string, logger *zap.SugaredLogger) (*HelmState, error) { var state HelmState state.BaseChartPath, _ = filepath.Abs(filepath.Dir(file)) @@ -111,16 +111,6 @@ func readFromYaml(content []byte, file string, logger *zap.SugaredLogger) (*Helm state.DeprecatedReleases = []ReleaseSpec{} } - releaseNameCounts := map[string]int{} - for _, r := range state.Releases { - releaseNameCounts[r.Name] += 1 - } - for name, c := range releaseNameCounts { - if c > 1 { - return nil, fmt.Errorf("invalid helmfile: duplicate release name found: there were %d releases named \"%s\"", c, name) - } - } - state.logger = logger return &state, nil @@ -144,7 +134,7 @@ func getRequiredEnv(name string) (string, error) { return "", fmt.Errorf("required env var `%s` is not set", name) } -func renderTemplateFileToBuffer(file string) (*bytes.Buffer, error) { +func RenderTemplateFileToBuffer(file string) (*bytes.Buffer, error) { content, err := ioutil.ReadFile(file) if err != nil { return nil, err @@ -712,7 +702,7 @@ func (state *HelmState) namespaceAndValuesFlags(helm helmexec.Interface, basePat if _, err := os.Stat(path); os.IsNotExist(err) { return nil, err } - yamlBuf, err := renderTemplateFileToBuffer(path) + yamlBuf, err := RenderTemplateFileToBuffer(path) if err != nil { return nil, err } diff --git a/state/state_test.go b/state/state_test.go index cc7562f2e..d48178d69 100644 --- a/state/state_test.go +++ b/state/state_test.go @@ -19,7 +19,7 @@ func TestReadFromYaml(t *testing.T) { namespace: mynamespace chart: mychart `) - state, err := readFromYaml(yamlContent, yamlFile, logger) + state, err := CreateFromYaml(yamlContent, yamlFile, logger) if err != nil { t.Errorf("unxpected error: %v", err) } @@ -42,7 +42,7 @@ func TestReadFromYaml_StrictUnmarshalling(t *testing.T) { namespace: mynamespace releases: mychart `) - _, err := readFromYaml(yamlContent, yamlFile, logger) + _, err := CreateFromYaml(yamlContent, yamlFile, logger) if err == nil { t.Error("expected an error for wrong key 'releases' which is not in struct") } @@ -54,7 +54,7 @@ func TestReadFromYaml_DeprecatedReleaseReferences(t *testing.T) { - name: myrelease chart: mychart `) - state, err := readFromYaml(yamlContent, yamlFile, logger) + state, err := CreateFromYaml(yamlContent, yamlFile, logger) if err != nil { t.Errorf("unxpected error: %v", err) } @@ -76,7 +76,7 @@ releases: - name: myrelease2 chart: mychart2 `) - _, err := readFromYaml(yamlContent, yamlFile, logger) + _, err := CreateFromYaml(yamlContent, yamlFile, logger) if err == nil { t.Error("expected error") } @@ -112,7 +112,7 @@ func TestReadFromYaml_FilterReleasesOnLabels(t *testing.T) { {LabelFilter{positiveLabels: [][]string{[]string{"tier", "frontend"}}, negativeLabels: [][]string{[]string{"foo", "bar"}}}, []bool{false, true, false}}, } - state, err := readFromYaml(yamlContent, yamlFile, logger) + state, err := CreateFromYaml(yamlContent, yamlFile, logger) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -151,7 +151,7 @@ func TestReadFromYaml_FilterNegatives(t *testing.T) { {LabelFilter{negativeLabels: [][]string{[]string{"stage", "pre"}, []string{"stage", "post"}}}, []bool{false, false, true}}, } - state, err := readFromYaml(yamlContent, yamlFile, logger) + state, err := CreateFromYaml(yamlContent, yamlFile, logger) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -164,29 +164,6 @@ func TestReadFromYaml_FilterNegatives(t *testing.T) { } } -// See https://github.com/roboll/helmfile/issues/193 -func TestReadFromYaml_DuplicateReleaseName(t *testing.T) { - yamlFile := "example/path/to/yaml/file" - yamlContent := []byte(`releases: -- name: myrelease1 - chart: mychart1 - labels: - stage: pre - foo: bar -- name: myrelease1 - chart: mychart2 - labels: - stage: post -`) - _, err := readFromYaml(yamlContent, yamlFile, logger) - if err == nil { - t.Error("error expected but not happened") - } - if err.Error() != "invalid helmfile: duplicate release name found: there were 2 releases named \"myrelease1\"" { - t.Errorf("unexpected error happened: %v", err) - } -} - func TestLabelParsing(t *testing.T) { cases := []struct { labelString string