From 75c99683b5fffe3cf7f0b8bd4c2710228066bed6 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Mon, 30 Dec 2019 09:52:00 -0800 Subject: [PATCH 1/4] AddRepo step that calls `helm repo add` [#26] As with some of the other commands, I'm not sure `--namespace` is relevant here. Just rolling with the "at worst it doesn't hurt" theory. --- docs/parameter_reference.md | 2 +- internal/run/addrepo.go | 48 +++++++++++++ internal/run/addrepo_test.go | 134 +++++++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 internal/run/addrepo.go create mode 100644 internal/run/addrepo_test.go diff --git a/docs/parameter_reference.md b/docs/parameter_reference.md index 07d8107..426dda9 100644 --- a/docs/parameter_reference.md +++ b/docs/parameter_reference.md @@ -5,7 +5,7 @@ |---------------------|-----------------|---------| | helm_command | string | Indicates the operation to perform. Recommended, but not required. Valid options are `upgrade`, `uninstall`, `lint`, and `help`. | | update_dependencies | boolean | Calls `helm dependency update` before running the main command. **Not currently implemented**; see [#25](https://github.com/pelotech/drone-helm3/issues/25).| -| helm_repos | list\ | Calls `helm repo add $repo` before running the main command. Each string should be formatted as `repo_name=https://repo.url/`. **Not currently implemented**; see [#26](https://github.com/pelotech/drone-helm3/issues/26). | +| helm_repos | list\ | Calls `helm repo add $repo` before running the main command. Each string should be formatted as `repo_name=https://repo.url/`. | | namespace | string | Kubernetes namespace to use for this operation. | | prefix | string | Expect environment variables to be prefixed with the given string. For more details, see "Using the prefix setting" below. **Not currently implemented**; see [#19](https://github.com/pelotech/drone-helm3/issues/19). | | debug | boolean | Generate debug output within drone-helm3 and pass `--debug` to all helm commands. Use with care, since the debug output may include secrets. | diff --git a/internal/run/addrepo.go b/internal/run/addrepo.go new file mode 100644 index 0000000..4dc326a --- /dev/null +++ b/internal/run/addrepo.go @@ -0,0 +1,48 @@ +package run + +import ( + "fmt" +) + +// AddRepo is an execution step that calls `helm repo add` when executed. +type AddRepo struct { + Name string + URL string + cmd cmd +} + +// Execute executes the `helm repo add` command. +func (a *AddRepo) Execute(_ Config) error { + return a.cmd.Run() +} + +// Prepare gets the AddRepo ready to execute. +func (a *AddRepo) Prepare(cfg Config) error { + if a.Name == "" { + return fmt.Errorf("repo name is required") + } + if a.URL == "" { + return fmt.Errorf("repo URL is required") + } + + args := make([]string, 0) + + if cfg.Namespace != "" { + args = append(args, "--namespace", cfg.Namespace) + } + if cfg.Debug { + args = append(args, "--debug") + } + + args = append(args, "repo", "add", a.Name, a.URL) + + a.cmd = command(helmBin, args...) + a.cmd.Stdout(cfg.Stdout) + a.cmd.Stderr(cfg.Stderr) + + if cfg.Debug { + fmt.Fprintf(cfg.Stderr, "Generated command: '%s'\n", a.cmd.String()) + } + + return nil +} diff --git a/internal/run/addrepo_test.go b/internal/run/addrepo_test.go new file mode 100644 index 0000000..867c039 --- /dev/null +++ b/internal/run/addrepo_test.go @@ -0,0 +1,134 @@ +package run + +import ( + "fmt" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/suite" + "strings" + "testing" +) + +type AddRepoTestSuite struct { + suite.Suite + ctrl *gomock.Controller + mockCmd *Mockcmd + originalCommand func(string, ...string) cmd + commandPath string + commandArgs []string +} + +func (suite *AddRepoTestSuite) BeforeTest(_, _ string) { + suite.ctrl = gomock.NewController(suite.T()) + suite.mockCmd = NewMockcmd(suite.ctrl) + + suite.originalCommand = command + command = func(path string, args ...string) cmd { + suite.commandPath = path + suite.commandArgs = args + return suite.mockCmd + } +} + +func (suite *AddRepoTestSuite) AfterTest(_, _ string) { + suite.ctrl.Finish() + command = suite.originalCommand +} + +func TestAddRepoTestSuite(t *testing.T) { + suite.Run(t, new(AddRepoTestSuite)) +} + +func (suite *AddRepoTestSuite) TestPrepareAndExecute() { + stdout := strings.Builder{} + stderr := strings.Builder{} + cfg := Config{ + Stdout: &stdout, + Stderr: &stderr, + } + a := AddRepo{ + Name: "edeath", + URL: "https://github.com/n_marks/e-death", + } + + suite.mockCmd.EXPECT(). + Stdout(&stdout). + Times(1) + suite.mockCmd.EXPECT(). + Stderr(&stderr). + Times(1) + + suite.Require().NoError(a.Prepare(cfg)) + suite.Equal(suite.commandPath, helmBin) + suite.Equal(suite.commandArgs, []string{"repo", "add", "edeath", "https://github.com/n_marks/e-death"}) + + suite.mockCmd.EXPECT(). + Run(). + Times(1) + + suite.Require().NoError(a.Execute(cfg)) + +} + +func (suite *AddRepoTestSuite) TestRequiredFields() { + // These aren't really expected, but allowing them gives clearer test-failure messages + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() + cfg := Config{} + a := AddRepo{ + Name: "idgen", + } + + err := a.Prepare(cfg) + suite.EqualError(err, "repo URL is required") + + a.Name = "" + a.URL = "https://github.com/n_marks/idgen" + + err = a.Prepare(cfg) + suite.EqualError(err, "repo name is required") +} + +func (suite *AddRepoTestSuite) TestNamespaceFlag() { + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() + cfg := Config{ + Namespace: "alliteration", + } + a := AddRepo{ + Name: "edeath", + URL: "https://github.com/theater_guy/e-death", + } + + suite.NoError(a.Prepare(cfg)) + suite.Equal(suite.commandPath, helmBin) + suite.Equal(suite.commandArgs, []string{"--namespace", "alliteration", + "repo", "add", "edeath", "https://github.com/theater_guy/e-death"}) +} + +func (suite *AddRepoTestSuite) TestDebugFlag() { + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() + + stderr := strings.Builder{} + + command = func(path string, args ...string) cmd { + suite.mockCmd.EXPECT(). + String(). + Return(fmt.Sprintf("%s %s", path, strings.Join(args, " "))) + + return suite.mockCmd + } + + cfg := Config{ + Debug: true, + Stderr: &stderr, + } + a := AddRepo{ + Name: "edeath", + URL: "https://github.com/the_bug/e-death", + } + + suite.Require().NoError(a.Prepare(cfg)) + suite.Equal(fmt.Sprintf("Generated command: '%s --debug "+ + "repo add edeath https://github.com/the_bug/e-death'\n", helmBin), stderr.String()) +} From 22e30fea5667e8f26d7378bcb39171487e33e8e4 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Mon, 30 Dec 2019 09:56:47 -0800 Subject: [PATCH 2/4] The prefix setting is implemented [#19,#9] Just something I noticed while resolving a merge conflict. The "write some docs" and "implement prefix" branches happened concurrently and didn't get re-coordinated. --- docs/parameter_reference.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/parameter_reference.md b/docs/parameter_reference.md index 703dd5e..52f3dd7 100644 --- a/docs/parameter_reference.md +++ b/docs/parameter_reference.md @@ -7,7 +7,7 @@ | update_dependencies | boolean | Calls `helm dependency update` before running the main command.| | helm_repos | list\ | Calls `helm repo add $repo` before running the main command. Each string should be formatted as `repo_name=https://repo.url/`. | | namespace | string | Kubernetes namespace to use for this operation. | -| prefix | string | Expect environment variables to be prefixed with the given string. For more details, see "Using the prefix setting" below. **Not currently implemented**; see [#19](https://github.com/pelotech/drone-helm3/issues/19). | +| prefix | string | Expect environment variables to be prefixed with the given string. For more details, see "Using the prefix setting" below. | | debug | boolean | Generate debug output within drone-helm3 and pass `--debug` to all helm commands. Use with care, since the debug output may include secrets. | ## Linting From 48b6b3f5b30f38d6c3800921eca74552617af2f1 Mon Sep 17 00:00:00 2001 From: Erin Call Date: Mon, 30 Dec 2019 11:57:19 -0800 Subject: [PATCH 3/4] Create AddRepo steps when there are repos to add [#26] --- internal/helm/config.go | 2 +- internal/helm/plan.go | 21 ++++++++++- internal/helm/plan_test.go | 75 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/internal/helm/config.go b/internal/helm/config.go index cf351ae..a4d1914 100644 --- a/internal/helm/config.go +++ b/internal/helm/config.go @@ -18,7 +18,7 @@ type Config struct { Command string `envconfig:"HELM_COMMAND"` // Helm command to run DroneEvent string `envconfig:"DRONE_BUILD_EVENT"` // Drone event that invoked this plugin. UpdateDependencies bool `split_words:"true"` // Call `helm dependency update` before the main command - Repos []string `envconfig:"HELM_REPOS"` // Call `helm repo add` before the main command + AddRepos []string `envconfig:"HELM_REPOS"` // Call `helm repo add` before the main command Prefix string `` // Prefix to use when looking up secret env vars Debug bool `` // Generate debug output and pass --debug to all helm commands Values string `` // Argument to pass to --set in applicable helm commands diff --git a/internal/helm/plan.go b/internal/helm/plan.go index ad2e5ae..303aa7c 100644 --- a/internal/helm/plan.go +++ b/internal/helm/plan.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/pelotech/drone-helm3/internal/run" "os" + "strings" ) const ( @@ -96,6 +97,7 @@ func (p *Plan) Execute() error { var upgrade = func(cfg Config) []Step { steps := initKube(cfg) + steps = append(steps, addRepos(cfg)...) if cfg.UpdateDependencies { steps = append(steps, depUpdate(cfg)...) } @@ -127,7 +129,7 @@ var uninstall = func(cfg Config) []Step { } var lint = func(cfg Config) []Step { - steps := make([]Step, 0) + steps := addRepos(cfg) if cfg.UpdateDependencies { steps = append(steps, depUpdate(cfg)...) } @@ -157,6 +159,23 @@ func initKube(cfg Config) []Step { } } +func addRepos(cfg Config) []Step { + steps := make([]Step, 0) + for _, repo := range cfg.AddRepos { + split := strings.SplitN(repo, "=", 2) + if len(split) != 2 { + fmt.Fprintf(cfg.Stderr, "Warning: skipping bad repo spec '%s'.\n", repo) + continue + } + steps = append(steps, &run.AddRepo{ + Name: split[0], + URL: split[1], + }) + } + + return steps +} + func depUpdate(cfg Config) []Step { return []Step{ &run.DepUpdate{ diff --git a/internal/helm/plan_test.go b/internal/helm/plan_test.go index 2cdde5c..f8db1bd 100644 --- a/internal/helm/plan_test.go +++ b/internal/helm/plan_test.go @@ -177,6 +177,17 @@ func (suite *PlanTestSuite) TestUpgradeWithUpdateDependencies() { suite.IsType(&run.DepUpdate{}, steps[1]) } +func (suite *PlanTestSuite) TestUpgradeWithAddRepos() { + cfg := Config{ + AddRepos: []string{ + "machine=https://github.com/harold_finch/themachine", + }, + } + steps := upgrade(cfg) + suite.Require().True(len(steps) > 1, "upgrade should generate at least two steps") + suite.IsType(&run.AddRepo{}, steps[1]) +} + func (suite *PlanTestSuite) TestUninstall() { cfg := Config{ KubeToken: "b2YgbXkgYWZmZWN0aW9u", @@ -268,6 +279,61 @@ func (suite *PlanTestSuite) TestDepUpdate() { suite.Equal(expected, update) } +func (suite *PlanTestSuite) TestAddRepos() { + cfg := Config{ + AddRepos: []string{ + "first=https://add.repos/one", + "second=https://add.repos/two", + }, + } + steps := addRepos(cfg) + suite.Require().Equal(2, len(steps), "addRepos should add one step per repo") + suite.Require().IsType(&run.AddRepo{}, steps[0]) + suite.Require().IsType(&run.AddRepo{}, steps[1]) + first := steps[0].(*run.AddRepo) + second := steps[1].(*run.AddRepo) + + suite.Equal(first.Name, "first") + suite.Equal(first.URL, "https://add.repos/one") + suite.Equal(second.Name, "second") + suite.Equal(second.URL, "https://add.repos/two") +} + +func (suite *PlanTestSuite) TestAddNoRepos() { + cfg := Config{ + AddRepos: []string{}, + } + steps := addRepos(cfg) + suite.Equal(0, len(steps), "adding no repos should take zero steps") +} + +func (suite *PlanTestSuite) TestAddReposWithMalformedRepoSpec() { + stderr := strings.Builder{} + cfg := Config{ + AddRepos: []string{ + "dwim", + }, + Stderr: &stderr, + } + steps := addRepos(cfg) + suite.Equal(len(steps), 0) + suite.Equal("Warning: skipping bad repo spec 'dwim'.\n", stderr.String()) +} + +func (suite *PlanTestSuite) TestAddReposWithEqualsInURL() { + cfg := Config{ + AddRepos: []string{ + "samaritan=https://github.com/arthur_claypool/samaritan?version=2.1", + }, + Stderr: &strings.Builder{}, + } + steps := addRepos(cfg) + suite.Require().Equal(1, len(steps)) + suite.Require().IsType(&run.AddRepo{}, steps[0]) + addRepo := steps[0].(*run.AddRepo) + suite.Equal("https://github.com/arthur_claypool/samaritan?version=2.1", addRepo.URL) +} + func (suite *PlanTestSuite) TestLint() { cfg := Config{ Chart: "./flow", @@ -291,6 +357,15 @@ func (suite *PlanTestSuite) TestLintWithUpdateDependencies() { suite.IsType(&run.DepUpdate{}, steps[0]) } +func (suite *PlanTestSuite) TestLintWithAddRepos() { + cfg := Config{ + AddRepos: []string{"friendczar=https://github.com/logan_pierce/friendczar"}, + } + steps := lint(cfg) + suite.Require().True(len(steps) > 0, "lint should return at least one step") + suite.IsType(&run.AddRepo{}, steps[0]) +} + func (suite *PlanTestSuite) TestDeterminePlanUpgradeCommand() { cfg := Config{ Command: "upgrade", From 499ab6877fca3df6f1426a766ad020cd28527eea Mon Sep 17 00:00:00 2001 From: Erin Call Date: Mon, 30 Dec 2019 13:24:57 -0800 Subject: [PATCH 4/4] Do repo error-checking in AddRepo.Prepare [#26] --- internal/helm/plan.go | 9 +------- internal/helm/plan_test.go | 41 ++---------------------------------- internal/run/addrepo.go | 18 ++++++++++------ internal/run/addrepo_test.go | 41 +++++++++++++++++++++--------------- 4 files changed, 38 insertions(+), 71 deletions(-) diff --git a/internal/helm/plan.go b/internal/helm/plan.go index 303aa7c..0a20969 100644 --- a/internal/helm/plan.go +++ b/internal/helm/plan.go @@ -4,7 +4,6 @@ import ( "fmt" "github.com/pelotech/drone-helm3/internal/run" "os" - "strings" ) const ( @@ -162,14 +161,8 @@ func initKube(cfg Config) []Step { func addRepos(cfg Config) []Step { steps := make([]Step, 0) for _, repo := range cfg.AddRepos { - split := strings.SplitN(repo, "=", 2) - if len(split) != 2 { - fmt.Fprintf(cfg.Stderr, "Warning: skipping bad repo spec '%s'.\n", repo) - continue - } steps = append(steps, &run.AddRepo{ - Name: split[0], - URL: split[1], + Repo: repo, }) } diff --git a/internal/helm/plan_test.go b/internal/helm/plan_test.go index f8db1bd..d4e4e82 100644 --- a/internal/helm/plan_test.go +++ b/internal/helm/plan_test.go @@ -293,45 +293,8 @@ func (suite *PlanTestSuite) TestAddRepos() { first := steps[0].(*run.AddRepo) second := steps[1].(*run.AddRepo) - suite.Equal(first.Name, "first") - suite.Equal(first.URL, "https://add.repos/one") - suite.Equal(second.Name, "second") - suite.Equal(second.URL, "https://add.repos/two") -} - -func (suite *PlanTestSuite) TestAddNoRepos() { - cfg := Config{ - AddRepos: []string{}, - } - steps := addRepos(cfg) - suite.Equal(0, len(steps), "adding no repos should take zero steps") -} - -func (suite *PlanTestSuite) TestAddReposWithMalformedRepoSpec() { - stderr := strings.Builder{} - cfg := Config{ - AddRepos: []string{ - "dwim", - }, - Stderr: &stderr, - } - steps := addRepos(cfg) - suite.Equal(len(steps), 0) - suite.Equal("Warning: skipping bad repo spec 'dwim'.\n", stderr.String()) -} - -func (suite *PlanTestSuite) TestAddReposWithEqualsInURL() { - cfg := Config{ - AddRepos: []string{ - "samaritan=https://github.com/arthur_claypool/samaritan?version=2.1", - }, - Stderr: &strings.Builder{}, - } - steps := addRepos(cfg) - suite.Require().Equal(1, len(steps)) - suite.Require().IsType(&run.AddRepo{}, steps[0]) - addRepo := steps[0].(*run.AddRepo) - suite.Equal("https://github.com/arthur_claypool/samaritan?version=2.1", addRepo.URL) + suite.Equal(first.Repo, "first=https://add.repos/one") + suite.Equal(second.Repo, "second=https://add.repos/two") } func (suite *PlanTestSuite) TestLint() { diff --git a/internal/run/addrepo.go b/internal/run/addrepo.go index 4dc326a..3382957 100644 --- a/internal/run/addrepo.go +++ b/internal/run/addrepo.go @@ -2,12 +2,12 @@ package run import ( "fmt" + "strings" ) // AddRepo is an execution step that calls `helm repo add` when executed. type AddRepo struct { - Name string - URL string + Repo string cmd cmd } @@ -18,13 +18,17 @@ func (a *AddRepo) Execute(_ Config) error { // Prepare gets the AddRepo ready to execute. func (a *AddRepo) Prepare(cfg Config) error { - if a.Name == "" { - return fmt.Errorf("repo name is required") + if a.Repo == "" { + return fmt.Errorf("repo is required") } - if a.URL == "" { - return fmt.Errorf("repo URL is required") + split := strings.SplitN(a.Repo, "=", 2) + if len(split) != 2 { + return fmt.Errorf("bad repo spec '%s'", a.Repo) } + name := split[0] + url := split[1] + args := make([]string, 0) if cfg.Namespace != "" { @@ -34,7 +38,7 @@ func (a *AddRepo) Prepare(cfg Config) error { args = append(args, "--debug") } - args = append(args, "repo", "add", a.Name, a.URL) + args = append(args, "repo", "add", name, url) a.cmd = command(helmBin, args...) a.cmd.Stdout(cfg.Stdout) diff --git a/internal/run/addrepo_test.go b/internal/run/addrepo_test.go index 867c039..ad42d06 100644 --- a/internal/run/addrepo_test.go +++ b/internal/run/addrepo_test.go @@ -46,8 +46,7 @@ func (suite *AddRepoTestSuite) TestPrepareAndExecute() { Stderr: &stderr, } a := AddRepo{ - Name: "edeath", - URL: "https://github.com/n_marks/e-death", + Repo: "edeath=https://github.com/n_marks/e-death", } suite.mockCmd.EXPECT(). @@ -58,8 +57,8 @@ func (suite *AddRepoTestSuite) TestPrepareAndExecute() { Times(1) suite.Require().NoError(a.Prepare(cfg)) - suite.Equal(suite.commandPath, helmBin) - suite.Equal(suite.commandArgs, []string{"repo", "add", "edeath", "https://github.com/n_marks/e-death"}) + suite.Equal(helmBin, suite.commandPath) + suite.Equal([]string{"repo", "add", "edeath", "https://github.com/n_marks/e-death"}, suite.commandArgs) suite.mockCmd.EXPECT(). Run(). @@ -69,23 +68,33 @@ func (suite *AddRepoTestSuite) TestPrepareAndExecute() { } -func (suite *AddRepoTestSuite) TestRequiredFields() { +func (suite *AddRepoTestSuite) TestPrepareRepoIsRequired() { // These aren't really expected, but allowing them gives clearer test-failure messages suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() cfg := Config{} - a := AddRepo{ - Name: "idgen", - } + a := AddRepo{} err := a.Prepare(cfg) - suite.EqualError(err, "repo URL is required") + suite.EqualError(err, "repo is required") +} - a.Name = "" - a.URL = "https://github.com/n_marks/idgen" +func (suite *AddRepoTestSuite) TestPrepareMalformedRepo() { + a := AddRepo{ + Repo: "dwim", + } + err := a.Prepare(Config{}) + suite.EqualError(err, "bad repo spec 'dwim'") +} - err = a.Prepare(cfg) - suite.EqualError(err, "repo name is required") +func (suite *AddRepoTestSuite) TestPrepareWithEqualSignInURL() { + suite.mockCmd.EXPECT().Stdout(gomock.Any()).AnyTimes() + suite.mockCmd.EXPECT().Stderr(gomock.Any()).AnyTimes() + a := AddRepo{ + Repo: "samaritan=https://github.com/arthur_claypool/samaritan?version=2.1", + } + suite.NoError(a.Prepare(Config{})) + suite.Contains(suite.commandArgs, "https://github.com/arthur_claypool/samaritan?version=2.1") } func (suite *AddRepoTestSuite) TestNamespaceFlag() { @@ -95,8 +104,7 @@ func (suite *AddRepoTestSuite) TestNamespaceFlag() { Namespace: "alliteration", } a := AddRepo{ - Name: "edeath", - URL: "https://github.com/theater_guy/e-death", + Repo: "edeath=https://github.com/theater_guy/e-death", } suite.NoError(a.Prepare(cfg)) @@ -124,8 +132,7 @@ func (suite *AddRepoTestSuite) TestDebugFlag() { Stderr: &stderr, } a := AddRepo{ - Name: "edeath", - URL: "https://github.com/the_bug/e-death", + Repo: "edeath=https://github.com/the_bug/e-death", } suite.Require().NoError(a.Prepare(cfg))