From cc17b9ab9eadcf0ce7def43a64c5d76ab612ca67 Mon Sep 17 00:00:00 2001 From: Aaron Prindle Date: Thu, 15 Apr 2021 11:59:52 -0700 Subject: [PATCH] add skaffold build --push flag What is the problem being solved? Fixes #5667, adding --push flag to skaffold build. This --push flag overrides any skaffold.yaml "build.local.push: false" configuration for a build allowing easier artifact pushing after local dev without having to modify yaml configuration. Why is this the best approach? This approach follows the style and convention for adding a flag that has been used prior for adding a flag used by a single command, similar to #4039. Additionally this approach does not change the skaffold build default push behaviour to be false, instead only taking precendence when the --push flag is set true which is the intended use case. What other approaches did you consider? N/A What side effects will this approach have? The addition of this single flag should not have side effects on skaffold functionally outside of --push, the default behavior for skaffold build will be preserved w/o --push specified. What future work remains to be done? N/A --- cmd/skaffold/app/cmd/build.go | 1 + cmd/skaffold/app/cmd/init_test.go | 2 +- docs/content/en/docs/references/cli/_index.md | 2 + pkg/skaffold/config/options.go | 1 + pkg/skaffold/config/types.go | 39 +++++++++++++ pkg/skaffold/config/types_test.go | 57 +++++++++++++++++++ pkg/skaffold/runner/build_deploy_test.go | 18 ++++++ pkg/skaffold/runner/new.go | 9 ++- 8 files changed, 126 insertions(+), 3 deletions(-) diff --git a/cmd/skaffold/app/cmd/build.go b/cmd/skaffold/app/cmd/build.go index 5b21c4cd026..038f58ec149 100644 --- a/cmd/skaffold/app/cmd/build.go +++ b/cmd/skaffold/app/cmd/build.go @@ -55,6 +55,7 @@ func NewCmdBuild() *cobra.Command { {Value: buildFormatFlag, Name: "output", Shorthand: "o", DefValue: defaultBuildFormatTemplate, Usage: "Used in conjunction with --quiet flag. " + buildFormatFlag.Usage()}, {Value: &buildOutputFlag, Name: "file-output", DefValue: "", Usage: "Filename to write build images to"}, {Value: &opts.DryRun, Name: "dry-run", DefValue: false, Usage: "Don't build images, just compute the tag for each artifact.", IsEnum: true}, + {Value: &opts.PushImages, Name: "push", DefValue: true, Usage: "Push the built images to the specified image repository.", IsEnum: true}, }). WithHouseKeepingMessages(). NoArgs(doBuild) diff --git a/cmd/skaffold/app/cmd/init_test.go b/cmd/skaffold/app/cmd/init_test.go index 119dda2f998..a6708445119 100644 --- a/cmd/skaffold/app/cmd/init_test.go +++ b/cmd/skaffold/app/cmd/init_test.go @@ -157,7 +157,7 @@ func TestFlagsToConfigVersion(t *testing.T) { // we ignore Skaffold options test.expectedConfig.Opts = capturedConfig.Opts - t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{})) + t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{}, cfg.BoolOrUndefined{})) }) } } diff --git a/docs/content/en/docs/references/cli/_index.md b/docs/content/en/docs/references/cli/_index.md index 7bbfe4b95a0..9379a9090ec 100644 --- a/docs/content/en/docs/references/cli/_index.md +++ b/docs/content/en/docs/references/cli/_index.md @@ -202,6 +202,7 @@ Options: -o, --output={{json .}}: Used in conjunction with --quiet flag. Format output with go-template. For full struct documentation, see https://godoc.org/github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags#BuildOutput -p, --profile=[]: Activate profiles by name (prefixed with `-` to disable a profile) --profile-auto-activation=true: Set to false to disable profile auto activation + --push=true: Push the built images to the specified image repository. -q, --quiet=false: Suppress the build output and print image built on success. See --output to format output. --remote-cache-dir='': Specify the location of the git repositories cache (default $HOME/.skaffold/repos) --rpc-http-port=50052: tcp port to expose event REST API over HTTP @@ -239,6 +240,7 @@ Env vars: * `SKAFFOLD_OUTPUT` (same as `--output`) * `SKAFFOLD_PROFILE` (same as `--profile`) * `SKAFFOLD_PROFILE_AUTO_ACTIVATION` (same as `--profile-auto-activation`) +* `SKAFFOLD_PUSH` (same as `--push`) * `SKAFFOLD_QUIET` (same as `--quiet`) * `SKAFFOLD_REMOTE_CACHE_DIR` (same as `--remote-cache-dir`) * `SKAFFOLD_RPC_HTTP_PORT` (same as `--rpc-http-port`) diff --git a/pkg/skaffold/config/options.go b/pkg/skaffold/config/options.go index 4e864497ef2..f050a2070cf 100644 --- a/pkg/skaffold/config/options.go +++ b/pkg/skaffold/config/options.go @@ -79,6 +79,7 @@ type SkaffoldOptions struct { DigestSource string WatchPollInterval int DefaultRepo StringOrUndefined + PushImages BoolOrUndefined CustomLabels []string TargetImages []string Profiles []string diff --git a/pkg/skaffold/config/types.go b/pkg/skaffold/config/types.go index 122ead411a8..e96db37b200 100644 --- a/pkg/skaffold/config/types.go +++ b/pkg/skaffold/config/types.go @@ -17,9 +17,48 @@ limitations under the License. package config import ( + "strconv" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" ) +// BoolOrUndefined holds the value of a flag of type `bool`, +// that's by default `undefined`. +// We use this instead of just `bool` to differentiate `undefined` +// and `[true|false]` bool values. +type BoolOrUndefined struct { + value *bool +} + +func (s *BoolOrUndefined) Type() string { + return "bool" +} + +func (s *BoolOrUndefined) Value() *bool { + return s.value +} + +func (s *BoolOrUndefined) Set(v string) error { + val, err := strconv.ParseBool(v) + if err != nil { + return err + } + s.value = &val + return nil +} + +func (s *BoolOrUndefined) SetNil() error { + s.value = nil + return nil +} + +func (s *BoolOrUndefined) String() string { + if s.value == nil { + return "" + } + return strconv.FormatBool(*s.value) +} + // StringOrUndefined holds the value of a flag of type `string`, // that's by default `undefined`. // We use this instead of just `string` to differentiate `undefined` diff --git a/pkg/skaffold/config/types_test.go b/pkg/skaffold/config/types_test.go index 3230e8c8cd0..d3496da8359 100644 --- a/pkg/skaffold/config/types_test.go +++ b/pkg/skaffold/config/types_test.go @@ -84,6 +84,63 @@ func TestStringOrUndefined(t *testing.T) { } } +func TestBoolOrUndefinedUsage(t *testing.T) { + var output bytes.Buffer + + cmd := &cobra.Command{} + cmd.Flags().Var(&BoolOrUndefined{}, "flag", "use it like this") + cmd.SetOutput(&output) + cmd.Usage() + + testutil.CheckDeepEqual(t, "Usage:\n\nFlags:\n --flag use it like this\n", output.String()) +} + +func TestBoolOrUndefined_SetNil(t *testing.T) { + var s BoolOrUndefined + s.Set("true") + testutil.CheckDeepEqual(t, "true", s.String()) + s.SetNil() + testutil.CheckDeepEqual(t, "", s.String()) + testutil.CheckDeepEqual(t, (*bool)(nil), s.value) + testutil.CheckDeepEqual(t, (*bool)(nil), s.Value()) +} + +func TestBoolOrUndefined(t *testing.T) { + tests := []struct { + description string + args []string + expected *bool + }{ + { + description: "undefined", + args: []string{}, + expected: nil, + }, + { + description: "set true", + args: []string{"--flag=true"}, + expected: util.BoolPtr(true), + }, + { + description: "set false", + args: []string{"--flag=false"}, + expected: util.BoolPtr(false), + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + var flag BoolOrUndefined + + cmd := &cobra.Command{} + cmd.Flags().Var(&flag, "flag", "") + cmd.SetArgs(test.args) + cmd.Execute() + + t.CheckDeepEqual(test.expected, flag.value) + }) + } +} + func TestMuted(t *testing.T) { tests := []struct { phases []string diff --git a/pkg/skaffold/runner/build_deploy_test.go b/pkg/skaffold/runner/build_deploy_test.go index 62df92f46a9..70acae77350 100644 --- a/pkg/skaffold/runner/build_deploy_test.go +++ b/pkg/skaffold/runner/build_deploy_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/client-go/tools/clientcmd/api" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/client" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -171,6 +172,23 @@ func TestBuildDryRun(t *testing.T) { }) } +func TestBuildPushFlag(t *testing.T) { + testutil.Run(t, "", func(t *testutil.T) { + testBench := &TestBench{} + artifacts := []*latest.Artifact{ + {ImageName: "img1"}, + {ImageName: "img2"}, + } + runner := createRunner(t, testBench, nil, artifacts, nil) + val := config.BoolOrUndefined{} + val.Set("true") + runner.runCtx.Opts.PushImages = val + _, err := runner.Build(context.Background(), ioutil.Discard, artifacts) + + t.CheckNoError(err) + }) +} + func TestDigestSources(t *testing.T) { artifacts := []*latest.Artifact{ {ImageName: "img1"}, diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 75e06d7a9b3..ff0f3e9dee1 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -188,10 +188,15 @@ func isImageLocal(runCtx *runcontext.RunContext, imageName string) (bool, error) cl := runCtx.GetCluster() var pushImages bool - if pipeline.Build.LocalBuild.Push == nil { + + switch { + case runCtx.Opts.PushImages.Value() != nil: + logrus.Debugf("push value set via skaffold build --push flag") + pushImages = *runCtx.Opts.PushImages.Value() + case pipeline.Build.LocalBuild.Push == nil: pushImages = cl.PushImages logrus.Debugf("push value not present, defaulting to %t because cluster.PushImages is %t", pushImages, cl.PushImages) - } else { + default: pushImages = *pipeline.Build.LocalBuild.Push } return !pushImages, nil