From 991bbf97b4c30755d75a466b6233d5503f14354d Mon Sep 17 00:00:00 2001 From: Erin Call Date: Tue, 17 Dec 2019 16:21:45 -0800 Subject: [PATCH 1/9] Create a Lint step [#3] Still need global flags and checks for mandatory settings, but the basic functionality is there. --- internal/run/lint.go | 28 +++++++++++++++++++ internal/run/lint_test.go | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 internal/run/lint.go create mode 100644 internal/run/lint_test.go diff --git a/internal/run/lint.go b/internal/run/lint.go new file mode 100644 index 0000000..85082c3 --- /dev/null +++ b/internal/run/lint.go @@ -0,0 +1,28 @@ +package run + +import ( +// "fmt" +) + +// Lint is an execution step that calls `helm lint` when executed. +type Lint struct { + Chart string + + cmd cmd +} + +// Execute executes the `helm lint` command. +func (l *Lint) Execute(_ Config) error { + return l.cmd.Run() +} + +// Prepare gets the Lint ready to execute. +func (l *Lint) Prepare(cfg Config) error { + args := []string{"lint", l.Chart} + + l.cmd = command(helmBin, args...) + l.cmd.Stdout(cfg.Stdout) + l.cmd.Stderr(cfg.Stderr) + + return nil +} diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go new file mode 100644 index 0000000..e86bcb5 --- /dev/null +++ b/internal/run/lint_test.go @@ -0,0 +1,58 @@ +package run + +import ( + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/suite" + "testing" +) + +type LintTestSuite struct { + suite.Suite + ctrl *gomock.Controller + mockCmd *Mockcmd + originalCommand func(string, ...string) cmd +} + +func (suite *LintTestSuite) BeforeTest(_, _ string) { + suite.ctrl = gomock.NewController(suite.T()) + suite.mockCmd = NewMockcmd(suite.ctrl) + + suite.originalCommand = command + command = func(path string, args ...string) cmd { return suite.mockCmd } +} + +func (suite *LintTestSuite) AfterTest(_, _ string) { + command = suite.originalCommand +} + +func TestLintTestSuite(t *testing.T) { + suite.Run(t, new(LintTestSuite)) +} + +func (suite *LintTestSuite) TestPrepareAndExecute() { + defer suite.ctrl.Finish() + + l := Lint{ + Chart: "./epic/mychart", + } + + command = func(path string, args ...string) cmd { + suite.Equal(helmBin, path) + suite.Equal([]string{"lint", "./epic/mychart"}, args) + + return suite.mockCmd + } + + suite.mockCmd.EXPECT(). + Stdout(gomock.Any()) + suite.mockCmd.EXPECT(). + Stderr(gomock.Any()) + suite.mockCmd.EXPECT(). + Run(). + Times(1) + + cfg := Config{} + err := l.Prepare(cfg) + suite.Require().Nil(err) + l.Execute(cfg) +} From 51800c18d76705184f23d3b850d2b2699a25676a Mon Sep 17 00:00:00 2001 From: Erin Call Date: Tue, 17 Dec 2019 16:41:09 -0800 Subject: [PATCH 2/9] Implement the various values flags in lint [#3] --- internal/run/lint.go | 14 +++++++++++++- internal/run/lint_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/run/lint.go b/internal/run/lint.go index 85082c3..b4093ac 100644 --- a/internal/run/lint.go +++ b/internal/run/lint.go @@ -18,7 +18,19 @@ func (l *Lint) Execute(_ Config) error { // Prepare gets the Lint ready to execute. func (l *Lint) Prepare(cfg Config) error { - args := []string{"lint", l.Chart} + args := []string{"lint"} + + if cfg.Values != "" { + args = append(args, "--set", cfg.Values) + } + if cfg.StringValues != "" { + args = append(args, "--set-string", cfg.StringValues) + } + for _, vFile := range cfg.ValuesFiles { + args = append(args, "--values", vFile) + } + + args = append(args, l.Chart) l.cmd = command(helmBin, args...) l.cmd.Stdout(cfg.Stdout) diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index e86bcb5..1c119f1 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -56,3 +56,35 @@ func (suite *LintTestSuite) TestPrepareAndExecute() { suite.Require().Nil(err) l.Execute(cfg) } + +func (suite *LintTestSuite) TestPrepareWithLintFlags() { + defer suite.ctrl.Finish() + + cfg := Config{ + Values: "width=5", + StringValues: "version=2.0", + ValuesFiles: []string{"/usr/local/underrides", "/usr/local/overrides"}, + } + + l := Lint{ + Chart: "./uk/top_40", + } + + command = func(path string, args ...string) cmd { + suite.Equal(helmBin, path) + suite.Equal([]string{"lint", + "--set", "width=5", + "--set-string", "version=2.0", + "--values", "/usr/local/underrides", + "--values", "/usr/local/overrides", + "./uk/top_40"}, args) + + return suite.mockCmd + } + + suite.mockCmd.EXPECT().Stdout(gomock.Any()) + suite.mockCmd.EXPECT().Stderr(gomock.Any()) + + err := l.Prepare(cfg) + suite.Require().Nil(err) +} From a6b7e06bd2c8cf6329e59674ae0bfcaf33cd7eba Mon Sep 17 00:00:00 2001 From: Erin Call Date: Tue, 17 Dec 2019 16:55:01 -0800 Subject: [PATCH 3/9] Implement the debug flag in lint [#3] --- internal/run/lint.go | 14 ++++++++++++-- internal/run/lint_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/internal/run/lint.go b/internal/run/lint.go index b4093ac..bdc4f8c 100644 --- a/internal/run/lint.go +++ b/internal/run/lint.go @@ -1,7 +1,7 @@ package run import ( -// "fmt" + "fmt" ) // Lint is an execution step that calls `helm lint` when executed. @@ -18,7 +18,13 @@ func (l *Lint) Execute(_ Config) error { // Prepare gets the Lint ready to execute. func (l *Lint) Prepare(cfg Config) error { - args := []string{"lint"} + args := make([]string, 0) + + if cfg.Debug { + args = append(args, "--debug") + } + + args = append(args, "lint") if cfg.Values != "" { args = append(args, "--set", cfg.Values) @@ -36,5 +42,9 @@ func (l *Lint) Prepare(cfg Config) error { l.cmd.Stdout(cfg.Stdout) l.cmd.Stderr(cfg.Stderr) + if cfg.Debug { + fmt.Fprintf(cfg.Stderr, "Generated command: '%s'\n", l.cmd.String()) + } + return nil } diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index 1c119f1..25a6f26 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -1,8 +1,10 @@ package run import ( + "fmt" "github.com/golang/mock/gomock" "github.com/stretchr/testify/suite" + "strings" "testing" ) @@ -88,3 +90,35 @@ func (suite *LintTestSuite) TestPrepareWithLintFlags() { err := l.Prepare(cfg) suite.Require().Nil(err) } + +func (suite *LintTestSuite) TestPrepareWithDebugFlag() { + defer suite.ctrl.Finish() + + stderr := strings.Builder{} + + cfg := Config{ + Debug: true, + Stderr: &stderr, + } + + l := Lint{ + Chart: "./scotland/top_40", + } + + command = func(path string, args ...string) cmd { + suite.mockCmd.EXPECT(). + String(). + Return(fmt.Sprintf("%s %s", path, strings.Join(args, " "))) + + return suite.mockCmd + } + + suite.mockCmd.EXPECT().Stdout(gomock.Any()) + suite.mockCmd.EXPECT().Stderr(gomock.Any()) + + err := l.Prepare(cfg) + suite.Require().Nil(err) + + want := fmt.Sprintf("Generated command: '%s --debug lint ./scotland/top_40'\n", helmBin) + suite.Equal(want, stderr.String()) +} From a6a2d6e6a342851f2cc962dbe8805efa0666218d Mon Sep 17 00:00:00 2001 From: Erin Call Date: Tue, 17 Dec 2019 17:00:34 -0800 Subject: [PATCH 4/9] Require a nonempty chart in Lint.Prepare [#3] --- internal/run/lint.go | 4 ++++ internal/run/lint_test.go | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/internal/run/lint.go b/internal/run/lint.go index bdc4f8c..aa18128 100644 --- a/internal/run/lint.go +++ b/internal/run/lint.go @@ -18,6 +18,10 @@ func (l *Lint) Execute(_ Config) error { // Prepare gets the Lint ready to execute. func (l *Lint) Prepare(cfg Config) error { + if l.Chart == "" { + return fmt.Errorf("chart is required") + } + args := make([]string, 0) if cfg.Debug { diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index 25a6f26..9f88598 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -59,6 +59,18 @@ func (suite *LintTestSuite) TestPrepareAndExecute() { l.Execute(cfg) } +func (suite *LintTestSuite) TestPrepareRequiresChart() { + // These aren't really expected, but allowing them gives clearer test-failure messages + suite.mockCmd.EXPECT().Stdout(gomock.Any()) + suite.mockCmd.EXPECT().Stderr(gomock.Any()) + + cfg := Config{} + l := Lint{} + + err := l.Prepare(cfg) + suite.EqualError(err, "chart is required", "Chart should be mandatory") +} + func (suite *LintTestSuite) TestPrepareWithLintFlags() { defer suite.ctrl.Finish() From 7e24756ad8da25018d61e98ee0a16275dccc0202 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Tue, 17 Dec 2019 17:14:39 -0800 Subject: [PATCH 5/9] Instantiate a Lint when cfg.Command is "lint" [#3] --- internal/helm/plan.go | 10 +++++++++- internal/helm/plan_test.go | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/helm/plan.go b/internal/helm/plan.go index ea184f3..61bc98e 100644 --- a/internal/helm/plan.go +++ b/internal/helm/plan.go @@ -62,7 +62,7 @@ func determineSteps(cfg Config) *func(Config) []Step { case "delete": panic("not implemented") case "lint": - panic("not implemented") + return &lint case "help": return &help default: @@ -116,6 +116,14 @@ var upgrade = func(cfg Config) []Step { return steps } +var lint = func(cfg Config) []Step { + lint := &run.Lint{ + Chart: cfg.Chart, + } + + return []Step{lint} +} + var help = func(cfg Config) []Step { help := &run.Help{} return []Step{help} diff --git a/internal/helm/plan_test.go b/internal/helm/plan_test.go index 716a27c..34708e6 100644 --- a/internal/helm/plan_test.go +++ b/internal/helm/plan_test.go @@ -137,6 +137,20 @@ func (suite *PlanTestSuite) TestUpgrade() { suite.Equal(expected, upgrade) } +func (suite *PlanTestSuite) TestLint() { + cfg := Config{ + Chart: "./flow", + } + + steps := lint(cfg) + suite.Equal(1, len(steps)) + + want := &run.Lint{ + Chart: "./flow", + } + suite.Equal(want, steps[0]) +} + func (suite *PlanTestSuite) TestDeterminePlanUpgradeCommand() { cfg := Config{ Command: "upgrade", @@ -156,6 +170,15 @@ func (suite *PlanTestSuite) TestDeterminePlanUpgradeFromDroneEvent() { } } +func (suite *PlanTestSuite) TestDeterminePlanLintCommand() { + cfg := Config{ + Command: "lint", + } + + stepsMaker := determineSteps(cfg) + suite.Same(&lint, stepsMaker) +} + func (suite *PlanTestSuite) TestDeterminePlanHelpCommand() { cfg := Config{ Command: "help", From 84ac01983869703f82589dbc110299d54d09ee33 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Wed, 18 Dec 2019 10:38:33 -0800 Subject: [PATCH 6/9] Add the --namespace flag in Lint.Prepare [#3] I don't know whether this is necessary; I'm just following drone-helm's lead. At worst, helm will accept the flag, so it's at least *safe* to include. --- internal/run/lint.go | 3 +++ internal/run/lint_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/internal/run/lint.go b/internal/run/lint.go index aa18128..620bc35 100644 --- a/internal/run/lint.go +++ b/internal/run/lint.go @@ -24,6 +24,9 @@ func (l *Lint) Prepare(cfg Config) error { args := make([]string, 0) + if cfg.Namespace != "" { + args = append(args, "--namespace", cfg.Namespace) + } if cfg.Debug { args = append(args, "--debug") } diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index 9f88598..b3881bd 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -134,3 +134,30 @@ func (suite *LintTestSuite) TestPrepareWithDebugFlag() { want := fmt.Sprintf("Generated command: '%s --debug lint ./scotland/top_40'\n", helmBin) suite.Equal(want, stderr.String()) } + +func (suite *LintTestSuite) TestPrepareWithNamespaceFlag() { + defer suite.ctrl.Finish() + + cfg := Config{ + Namespace: "table-service", + } + + l := Lint{ + Chart: "./wales/top_40", + } + + actual := []string{} + command = func(path string, args ...string) cmd { + actual = args + return suite.mockCmd + } + + suite.mockCmd.EXPECT().Stdout(gomock.Any()) + suite.mockCmd.EXPECT().Stderr(gomock.Any()) + + err := l.Prepare(cfg) + suite.Require().Nil(err) + + expected := []string{"--namespace", "table-service", "lint", "./wales/top_40"} + suite.Equal(expected, actual) +} From b93917c857d83eb4b5d580be3d943b1dbd83edd8 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Thu, 19 Dec 2019 10:21:11 -0800 Subject: [PATCH 7/9] Use better expectations in lint_test [#3] The tests need to allow calls to Stdout/Stderr so they don't get "Unexpected call" errors from gomock, but these tests aren't meant to assert that the calls actually happened. Using .AnyTimes allows 0 or more calls. --- internal/run/lint_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index b3881bd..feb6d5f 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -61,8 +61,8 @@ func (suite *LintTestSuite) TestPrepareAndExecute() { func (suite *LintTestSuite) TestPrepareRequiresChart() { // These aren't really expected, but allowing them gives clearer test-failure messages - suite.mockCmd.EXPECT().Stdout(gomock.Any()) - suite.mockCmd.EXPECT().Stderr(gomock.Any()) + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() cfg := Config{} l := Lint{} @@ -96,8 +96,8 @@ func (suite *LintTestSuite) TestPrepareWithLintFlags() { return suite.mockCmd } - suite.mockCmd.EXPECT().Stdout(gomock.Any()) - suite.mockCmd.EXPECT().Stderr(gomock.Any()) + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() err := l.Prepare(cfg) suite.Require().Nil(err) @@ -126,7 +126,7 @@ func (suite *LintTestSuite) TestPrepareWithDebugFlag() { } suite.mockCmd.EXPECT().Stdout(gomock.Any()) - suite.mockCmd.EXPECT().Stderr(gomock.Any()) + suite.mockCmd.EXPECT().Stderr(&stderr) err := l.Prepare(cfg) suite.Require().Nil(err) @@ -152,8 +152,8 @@ func (suite *LintTestSuite) TestPrepareWithNamespaceFlag() { return suite.mockCmd } - suite.mockCmd.EXPECT().Stdout(gomock.Any()) - suite.mockCmd.EXPECT().Stderr(gomock.Any()) + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() err := l.Prepare(cfg) suite.Require().Nil(err) From 30e1e3b99f8e1e475ce40bc8707a32e7f51e62b1 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Thu, 19 Dec 2019 10:26:33 -0800 Subject: [PATCH 8/9] Assert that Lint.Prepare sets cmd.Stdout/Stderr [#3] --- internal/run/lint_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/run/lint_test.go b/internal/run/lint_test.go index feb6d5f..9d683b4 100644 --- a/internal/run/lint_test.go +++ b/internal/run/lint_test.go @@ -34,9 +34,16 @@ func TestLintTestSuite(t *testing.T) { func (suite *LintTestSuite) TestPrepareAndExecute() { defer suite.ctrl.Finish() + stdout := strings.Builder{} + stderr := strings.Builder{} + l := Lint{ Chart: "./epic/mychart", } + cfg := Config{ + Stdout: &stdout, + Stderr: &stderr, + } command = func(path string, args ...string) cmd { suite.Equal(helmBin, path) @@ -46,14 +53,13 @@ func (suite *LintTestSuite) TestPrepareAndExecute() { } suite.mockCmd.EXPECT(). - Stdout(gomock.Any()) + Stdout(&stdout) suite.mockCmd.EXPECT(). - Stderr(gomock.Any()) + Stderr(&stderr) suite.mockCmd.EXPECT(). Run(). Times(1) - cfg := Config{} err := l.Prepare(cfg) suite.Require().Nil(err) l.Execute(cfg) From c033c8c45e719981d7d5d1c7855197a4fc75fee5 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Thu, 19 Dec 2019 11:09:39 -0800 Subject: [PATCH 9/9] Format the Lint struct non-weirdly [#3] I thought it was a golang style convention to put a blank line between public and private struct fields, but apparently I imagined that. --- internal/run/lint.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/run/lint.go b/internal/run/lint.go index 620bc35..e2843ca 100644 --- a/internal/run/lint.go +++ b/internal/run/lint.go @@ -7,8 +7,7 @@ import ( // Lint is an execution step that calls `helm lint` when executed. type Lint struct { Chart string - - cmd cmd + cmd cmd } // Execute executes the `helm lint` command.