From 68e6028e1502822921d8905193160af21fb7856e Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Wed, 28 Sep 2022 23:05:24 +0000 Subject: [PATCH 1/6] core/cli: Add string ptr to flag types --- internal/pkg/flag/flag_string_ptr.go | 65 ++++++++++++++++++++ internal/pkg/flag/flag_string_ptr_test.go | 75 +++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 internal/pkg/flag/flag_string_ptr.go create mode 100644 internal/pkg/flag/flag_string_ptr_test.go diff --git a/internal/pkg/flag/flag_string_ptr.go b/internal/pkg/flag/flag_string_ptr.go new file mode 100644 index 00000000000..9ff729a04f2 --- /dev/null +++ b/internal/pkg/flag/flag_string_ptr.go @@ -0,0 +1,65 @@ +package flag + +import ( + "github.com/posener/complete" +) + +// -- StringPtrVar and stringValue +type StringPtrVar struct { + Name string + Aliases []string + Usage string + Default string + Hidden bool + EnvVar string + Target **string + Completion complete.Predictor + SetHook func(val string) +} + +func (f *Set) StringPtrVar(i *StringPtrVar) { + f.VarFlag(&VarFlag{ + Name: i.Name, + Aliases: i.Aliases, + Default: i.Default, + Usage: i.Usage, + EnvVar: i.EnvVar, + Value: newStringPtr(i, i.Target, i.Hidden), + Completion: i.Completion, + }) +} + +type stringPtrValue struct { + v *StringPtrVar + hidden bool + target **string +} + +func newStringPtr(v *StringPtrVar, target **string, hidden bool) *stringPtrValue { + return &stringPtrValue{ + v: v, + hidden: hidden, + target: target, + } +} + +func (s *stringPtrValue) Set(val string) error { + *s.target = &val + + if s.v.SetHook != nil { + s.v.SetHook(val) + } + + return nil +} + +func (s *stringPtrValue) String() string { + if *s.target != nil { + return **s.target + } + return s.v.Default +} + +func (s *stringPtrValue) Get() interface{} { return s.target } +func (s *stringPtrValue) Example() string { return "string" } +func (s *stringPtrValue) Hidden() bool { return s.hidden } diff --git a/internal/pkg/flag/flag_string_ptr_test.go b/internal/pkg/flag/flag_string_ptr_test.go new file mode 100644 index 00000000000..5a50b8ae39d --- /dev/null +++ b/internal/pkg/flag/flag_string_ptr_test.go @@ -0,0 +1,75 @@ +package flag + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestStringPtrVar(t *testing.T) { + cases := map[string]struct { + input *string + expected *string + omitValue bool // e.g. -poll instead of -poll=true + shouldErr bool + }{ + "omitted": { + expected: nil, + }, + // specifying -flag without a value should fail + "no value set": { + omitValue: true, + shouldErr: true, + }, + "value passed in": { + input: strPtr("blammo"), + expected: strPtr("blammo"), + }, + // empty is equivilant to a user supplying -flag="", and not a scenario + // where the flag is simply omittied. + "empty string as input": { + input: strPtr(""), + expected: strPtr(""), + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + var valA *string + var valB string + sets := NewSets() + { + set := sets.NewSet("A") + set.StringPtrVar(&StringPtrVar{ + Name: "a", + Target: &valA, + }) + } + { + // borrowed from string_slice_test, just to have another input + set := sets.NewSet("B") + set.StringVar(&StringVar{ + Name: "b", + Target: &valB, + }) + } + + var err error + inputs := []string{"-b=somevalueB"} + if c.input != nil { + inputs = append(inputs, "-a="+*c.input) + } + if c.omitValue == true { + inputs = append(inputs, "-a") + } + err = sets.Parse(inputs) + if c.shouldErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + assert.Equal(t, c.expected, valA) + }) + } +} From 30fb1e040152f56cad070046aaf72255603815a3 Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Wed, 28 Sep 2022 23:14:39 +0000 Subject: [PATCH 2/6] core/cli: Update runner profile command for proper upsert Before this commit when updating a runner profile with waypoint runner profile set, the command would reset all unset configs to their defaults. This commit checks first if the flag exists before setting it to the value in the flag, otherwise we prefer the already set configuration. --- internal/cli/runner_profile_set.go | 87 +++++++++++++++++++----------- 1 file changed, 57 insertions(+), 30 deletions(-) diff --git a/internal/cli/runner_profile_set.go b/internal/cli/runner_profile_set.go index c08a8fdf766..759a8717e8b 100644 --- a/internal/cli/runner_profile_set.go +++ b/internal/cli/runner_profile_set.go @@ -25,13 +25,14 @@ type RunnerProfileSetCommand struct { *baseCommand //TODO(XX): after `-env-vars` as a slice is deprecated, rename flagEnvVar to flagEnvVars flagName string - flagOCIUrl string + flagOCIUrl *string flagEnvVar map[string]string flagEnvVars []string - flagPluginType string + flagPluginType *string flagPluginConfig string flagDefault *bool - flagTargetRunnerId string + flagTargetRunnerAny *bool + flagTargetRunnerId *string flagTargetRunnerLabels map[string]string } @@ -104,14 +105,18 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { } // Set target runner for profile - if c.flagTargetRunnerId != "" { + if c.flagTargetRunnerId != nil { od.TargetRunner = &pb.Ref_Runner{ Target: &pb.Ref_Runner_Id{ Id: &pb.Ref_RunnerId{ - Id: c.flagTargetRunnerId, + Id: *c.flagTargetRunnerId, }, }, } + if c.flagTargetRunnerAny != nil { + c.ui.Output("Both -target-runner-id and -target-runner-any detected, only one can be set at a time. ID takes priority.", + terminal.WithWarningStyle()) + } if c.flagTargetRunnerLabels != nil { c.ui.Output("Both -target-runner-id and -target-runner-label detected, only one can be set at a time. ID takes priority.", terminal.WithWarningStyle()) @@ -124,7 +129,11 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { }, }, } - } else { + if c.flagTargetRunnerAny != nil { + c.ui.Output("Both -target-runner-id and -target-runner-any detected, only one can be set at a time. ID takes priority.", + terminal.WithWarningStyle()) + } + } else if c.flagTargetRunnerAny != nil && *c.flagTargetRunnerAny { od.TargetRunner = &pb.Ref_Runner{Target: &pb.Ref_Runner_Any{}} } @@ -189,8 +198,13 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { } } - od.OciUrl = c.flagOCIUrl - od.EnvironmentVariables = map[string]string{} + if od.OciUrl == "" { + od.OciUrl = installutil.DefaultODRImage + } + if c.flagOCIUrl != nil { + od.OciUrl = *c.flagOCIUrl + } + if c.flagDefault != nil { od.Default = *c.flagDefault } @@ -198,7 +212,7 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { if c.flagEnvVars != nil { //TODO(XX): Deprecate -env-vars and this logic c.ui.Output( - "Flag '-env-vars' is deprecated, please use flag `-env-var=k=v`", + "Flag '-env-vars' is deprecated, please use flag '-env-var=k=v'", terminal.WithWarningStyle(), ) for _, kv := range c.flagEnvVars { @@ -207,20 +221,29 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { od.EnvironmentVariables[kv[:idx]] = kv[idx+1:] } } - } else { - od.EnvironmentVariables = c.flagEnvVar } - if c.flagPluginType == "" { - c.ui.Output( - "Flag '-plugin-type' must be set to a valid plugin type like 'docker' or 'kubernetes'.\n\n%s", - c.Help(), - terminal.WithErrorStyle(), - ) - return 1 + if c.flagEnvVar != nil { + for k, v := range c.flagEnvVar { + if v == "" { + delete(od.EnvironmentVariables, k) + } else { + od.EnvironmentVariables[k] = v + } + } } - od.PluginType = c.flagPluginType + if c.flagPluginType != nil || od.PluginType == "" { + if *c.flagPluginType == "" { + c.ui.Output( + "Flag '-plugin-type' must be set to a valid plugin type like 'docker' or 'kubernetes'.\n\n%s", + c.Help(), + terminal.WithErrorStyle(), + ) + return 1 + } + od.PluginType = *c.flagPluginType + } // Upsert _, err := c.project.Client().UpsertOnDemandRunnerConfig(ctx, &pb.UpsertOnDemandRunnerConfigRequest{ @@ -255,7 +278,7 @@ func (c *RunnerProfileSetCommand) Flags() *flag.Sets { Usage: "The name of an existing runner profile to update.", }) - f.StringVar(&flag.StringVar{ + f.StringPtrVar(&flag.StringPtrVar{ Name: "oci-url", Target: &c.flagOCIUrl, Default: installutil.DefaultODRImage, @@ -276,11 +299,10 @@ func (c *RunnerProfileSetCommand) Flags() *flag.Sets { Usage: "DEPRECATED. Please see `-env-var`.", }) - f.StringVar(&flag.StringVar{ - Name: "plugin-type", - Target: &c.flagPluginType, - Default: "", - Usage: "The type of the plugin to launch for the on-demand runner, such as aws-ecs, kubernetes, etc.", + f.StringPtrVar(&flag.StringPtrVar{ + Name: "plugin-type", + Target: &c.flagPluginType, + Usage: "The type of the plugin to launch for the on-demand runner, such as aws-ecs, kubernetes, etc.", }) f.StringVar(&flag.StringVar{ @@ -299,11 +321,10 @@ func (c *RunnerProfileSetCommand) Flags() *flag.Sets { "otherwise specify its own remote runner.", }) - f.StringVar(&flag.StringVar{ - Name: "target-runner-id", - Target: &c.flagTargetRunnerId, - Default: "", - Usage: "ID of the runner to target for this remote runner profile.", + f.StringPtrVar(&flag.StringPtrVar{ + Name: "target-runner-id", + Target: &c.flagTargetRunnerId, + Usage: "ID of the runner to target for this remote runner profile.", }) f.StringMapVar(&flag.StringMapVar{ @@ -312,6 +333,12 @@ func (c *RunnerProfileSetCommand) Flags() *flag.Sets { Usage: "Labels on the runner to target for this remote runner profile. " + "e.g. `-target-runner-label=k=v`. Can be specified multiple times.", }) + + f.BoolPtrVar(&flag.BoolPtrVar{ + Name: "target-runner-any", + Target: &c.flagTargetRunnerAny, + Usage: "Set profile to target any available runner.", + }) }) } From 622ed3251703e24ebb6046524f7242a9a0862978 Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Wed, 28 Sep 2022 23:27:40 +0000 Subject: [PATCH 3/6] core/cli: Set target runner to any when initializing profile --- internal/cli/runner_profile_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cli/runner_profile_set.go b/internal/cli/runner_profile_set.go index 759a8717e8b..7c730d03816 100644 --- a/internal/cli/runner_profile_set.go +++ b/internal/cli/runner_profile_set.go @@ -133,7 +133,7 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { c.ui.Output("Both -target-runner-id and -target-runner-any detected, only one can be set at a time. ID takes priority.", terminal.WithWarningStyle()) } - } else if c.flagTargetRunnerAny != nil && *c.flagTargetRunnerAny { + } else if od.TargetRunner == nil || (c.flagTargetRunnerAny != nil && *c.flagTargetRunnerAny) { od.TargetRunner = &pb.Ref_Runner{Target: &pb.Ref_Runner_Any{}} } From 4c38c78f16db4dbffc9fafc4487425adf154a71f Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Wed, 28 Sep 2022 23:28:12 +0000 Subject: [PATCH 4/6] Add changelog --- .changelog/3854.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changelog/3854.txt diff --git a/.changelog/3854.txt b/.changelog/3854.txt new file mode 100644 index 00000000000..ccf44ceb90e --- /dev/null +++ b/.changelog/3854.txt @@ -0,0 +1,7 @@ +```release-note:improvement +cli: Setting configurations in a runner profile no longer resets unspecified configuration +``` + +```release-note:improvement +cli: A new `-runner-target-any` flag has been added to runner profile set to allow users to specify targeting any runner. +``` From 223b6585e622d09329e065b541a8bd6688940436 Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Wed, 28 Sep 2022 23:30:44 +0000 Subject: [PATCH 5/6] docs: update website with new flag --- website/content/commands/runner-profile-set.mdx | 1 + 1 file changed, 1 insertion(+) diff --git a/website/content/commands/runner-profile-set.mdx b/website/content/commands/runner-profile-set.mdx index c1101aa3aba..b144530eadf 100644 --- a/website/content/commands/runner-profile-set.mdx +++ b/website/content/commands/runner-profile-set.mdx @@ -44,5 +44,6 @@ lifecycle operation. - `-default` - Indicates that this remote runner profile should be the default for any project that doesn't otherwise specify its own remote runner. - `-target-runner-id=` - ID of the runner to target for this remote runner profile. - `-target-runner-label=` - Labels on the runner to target for this remote runner profile. e.g. `-target-runner-label=k=v`. Can be specified multiple times. +- `-target-runner-any` - Set profile to target any available runner. @include "commands/runner-profile-set_more.mdx" From cebe713934a9272256832c499c9b420a143461cc Mon Sep 17 00:00:00 2001 From: Britt Gresham Date: Thu, 29 Sep 2022 22:48:53 +0000 Subject: [PATCH 6/6] Update warning message when both labels and any specified --- internal/cli/runner_profile_set.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cli/runner_profile_set.go b/internal/cli/runner_profile_set.go index 7c730d03816..e7ab8c5a202 100644 --- a/internal/cli/runner_profile_set.go +++ b/internal/cli/runner_profile_set.go @@ -130,7 +130,7 @@ func (c *RunnerProfileSetCommand) Run(args []string) int { }, } if c.flagTargetRunnerAny != nil { - c.ui.Output("Both -target-runner-id and -target-runner-any detected, only one can be set at a time. ID takes priority.", + c.ui.Output("Both -target-runner-label and -target-runner-any detected, only one can be set at a time. Labels take priority.", terminal.WithWarningStyle()) } } else if od.TargetRunner == nil || (c.flagTargetRunnerAny != nil && *c.flagTargetRunnerAny) {