From 8573b0298161e5767aeaa4e1911d34d0bf6f1531 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 8 Oct 2018 08:36:58 -0400 Subject: [PATCH 1/5] skipPush -> push --- examples/annotated-skaffold.yaml | 2 +- pkg/skaffold/build/local/local_test.go | 4 ++-- pkg/skaffold/build/local/types.go | 6 +++--- pkg/skaffold/schema/latest/config.go | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/examples/annotated-skaffold.yaml b/examples/annotated-skaffold.yaml index 92cbc412e76..4e31701a1a9 100644 --- a/examples/annotated-skaffold.yaml +++ b/examples/annotated-skaffold.yaml @@ -77,7 +77,7 @@ build: # `useBuildkit` can also be set to activate the experimental BuildKit feature. # # local: - # skipPush: true + # push: false # useDockerCLI: false # useBuildkit: false diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index 04d8cbea98f..8c959421f6b 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -80,7 +80,7 @@ func TestLocalRun(t *testing.T) { description: "single build", out: ioutil.Discard, config: &latest.LocalBuild{ - SkipPush: util.BoolPtr(false), + Push: util.BoolPtr(false), }, artifacts: []*latest.Artifact{ { @@ -104,7 +104,7 @@ func TestLocalRun(t *testing.T) { description: "subset build", out: ioutil.Discard, config: &latest.LocalBuild{ - SkipPush: util.BoolPtr(true), + Push: util.BoolPtr(true), }, tagger: &tag.ChecksumTagger{}, artifacts: []*latest.Artifact{ diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go index d693c849bcc..244ae7a145c 100644 --- a/pkg/skaffold/build/local/types.go +++ b/pkg/skaffold/build/local/types.go @@ -48,11 +48,11 @@ func NewBuilder(cfg *latest.LocalBuild, kubeContext string) (*Builder, error) { localCluster := kubeContext == constants.DefaultMinikubeContext || kubeContext == constants.DefaultDockerForDesktopContext var pushImages bool - if cfg.SkipPush == nil { - logrus.Debugf("skipPush value not present. defaulting to cluster default %t (minikube=true, d4d=true, gke=false)", localCluster) + if cfg.Push == nil { pushImages = !localCluster + logrus.Debugf("push value not present, defaulting to %t because localCluster is %t", pushImages, localCluster) } else { - pushImages = !*cfg.SkipPush + pushImages = *cfg.Push } return &Builder{ diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 5d32e57b180..949bbb43579 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -86,7 +86,7 @@ type BuildType struct { // LocalBuild contains the fields needed to do a build on the local docker daemon // and optionally push to a repository. type LocalBuild struct { - SkipPush *bool `yaml:"skipPush,omitempty"` + Push *bool `yaml:"push,omitempty"` UseDockerCLI bool `yaml:"useDockerCLI,omitempty"` UseBuildkit bool `yaml:"useBuildkit,omitempty"` } From 70954c848d0c453622b8e81f67867297e58c8796 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 8 Oct 2018 08:39:14 -0400 Subject: [PATCH 2/5] add note about default push behavior in annotated skaffold.yaml --- examples/annotated-skaffold.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/annotated-skaffold.yaml b/examples/annotated-skaffold.yaml index 4e31701a1a9..e45c5a63a3d 100644 --- a/examples/annotated-skaffold.yaml +++ b/examples/annotated-skaffold.yaml @@ -77,6 +77,7 @@ build: # `useBuildkit` can also be set to activate the experimental BuildKit feature. # # local: + # false by default for local clusters, true for remote clusters # push: false # useDockerCLI: false # useBuildkit: false From e3b0a6942797a6b304e62ebacb77e633cdf82cb8 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 8 Oct 2018 09:38:10 -0400 Subject: [PATCH 3/5] upgrade logic for skipPush -> push config change --- examples/annotated-skaffold.yaml | 3 +- integration/examples/annotated-skaffold.yaml | 3 +- pkg/skaffold/schema/v1alpha3/upgrade.go | 14 ++++- pkg/skaffold/schema/v1alpha3/upgrade_test.go | 57 ++++++++++++++++++++ skaffold.yaml | 2 +- 5 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 pkg/skaffold/schema/v1alpha3/upgrade_test.go diff --git a/examples/annotated-skaffold.yaml b/examples/annotated-skaffold.yaml index e45c5a63a3d..92cbc412e76 100644 --- a/examples/annotated-skaffold.yaml +++ b/examples/annotated-skaffold.yaml @@ -77,8 +77,7 @@ build: # `useBuildkit` can also be set to activate the experimental BuildKit feature. # # local: - # false by default for local clusters, true for remote clusters - # push: false + # skipPush: true # useDockerCLI: false # useBuildkit: false diff --git a/integration/examples/annotated-skaffold.yaml b/integration/examples/annotated-skaffold.yaml index f7c3e550271..0b35d3af0d5 100644 --- a/integration/examples/annotated-skaffold.yaml +++ b/integration/examples/annotated-skaffold.yaml @@ -77,7 +77,8 @@ build: # `useBuildkit` can also be set to activate the experimental BuildKit feature. # # local: - # skipPush: true + # false by default for local clusters, true for remote clusters + # push: false # useDockerCLI: false # useBuildkit: false diff --git a/pkg/skaffold/schema/v1alpha3/upgrade.go b/pkg/skaffold/schema/v1alpha3/upgrade.go index 287f2dcf6af..f4cd3f920b4 100644 --- a/pkg/skaffold/schema/v1alpha3/upgrade.go +++ b/pkg/skaffold/schema/v1alpha3/upgrade.go @@ -38,13 +38,18 @@ func (config *SkaffoldPipeline) Upgrade() (util.VersionedConfig, error) { if err := convert(config.Profiles, &newProfiles); err != nil { return nil, errors.Wrap(err, "converting new profile") } + for i, oldProfile := range config.Profiles { + convertBuild(oldProfile.Build, newProfiles[i].Build) + } } // convert Build (should be the same) var newBuild next.BuildConfig - if err := convert(config.Build, &newBuild); err != nil { + oldBuild := config.Build + if err := convert(oldBuild, &newBuild); err != nil { return nil, errors.Wrap(err, "converting new build") } + convertBuild(oldBuild, newBuild) return &next.SkaffoldPipeline{ APIVersion: next.Version, @@ -54,6 +59,13 @@ func (config *SkaffoldPipeline) Upgrade() (util.VersionedConfig, error) { }, nil } +func convertBuild(oldBuild BuildConfig, newBuild next.BuildConfig) { + if oldBuild.LocalBuild != nil && oldBuild.LocalBuild.SkipPush != nil { + push := !*oldBuild.LocalBuild.SkipPush + newBuild.LocalBuild.Push = &push + } +} + func convert(old interface{}, new interface{}) error { o, err := json.Marshal(old) if err != nil { diff --git a/pkg/skaffold/schema/v1alpha3/upgrade_test.go b/pkg/skaffold/schema/v1alpha3/upgrade_test.go new file mode 100644 index 00000000000..ab53cd77e8d --- /dev/null +++ b/pkg/skaffold/schema/v1alpha3/upgrade_test.go @@ -0,0 +1,57 @@ +package v1alpha3 + +import ( + "testing" + v1alpha4 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" +) + +func TestBuildUpgrade(t *testing.T) { + old := `apiVersion: skaffold/v1alpha3 +kind: Config +build: + local: + skipPush: false +profiles: + - name: testEnv1 + build: + local: + skipPush: true + - name: testEnv2 + build: + local: + skipPush: false +` + pipeline := NewSkaffoldPipeline() + err := pipeline.Parse([]byte(old), true) + if err != nil { + t.Errorf("unexpected error during parsing old config: %v", err) + } + + upgraded, err := pipeline.Upgrade() + if err != nil { + t.Errorf("unexpected error during upgrade: %v", err) + } + + upgradedPipeline := upgraded.(*v1alpha4.SkaffoldPipeline) + + if upgradedPipeline.Build.LocalBuild == nil { + t.Errorf("expected build.local to be not nil") + } + if upgradedPipeline.Build.LocalBuild.Push != nil && *upgradedPipeline.Build.LocalBuild.Push != true { + t.Errorf("expected build.local.push to be true but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) + } + + if upgradedPipeline.Profiles[0].Build.LocalBuild == nil { + t.Errorf("expected profiles[0].build.local to be not nil") + } + if upgradedPipeline.Profiles[0].Build.LocalBuild.Push != nil && *upgradedPipeline.Profiles[0].Build.LocalBuild.Push != false { + t.Errorf("expected profiles[0].build.local.push to be false but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) + } + + if upgradedPipeline.Profiles[1].Build.LocalBuild == nil { + t.Errorf("expected profiles[1].build.local to be not nil") + } + if upgradedPipeline.Profiles[1].Build.LocalBuild.Push != nil && *upgradedPipeline.Profiles[1].Build.LocalBuild.Push != true { + t.Errorf("expected profiles[1].build.local.push to be true but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) + } +} diff --git a/skaffold.yaml b/skaffold.yaml index 2979b09396f..a1a66d0522a 100644 --- a/skaffold.yaml +++ b/skaffold.yaml @@ -2,7 +2,7 @@ apiVersion: skaffold/v1alpha3 kind: Config build: artifacts: - - image: gcr.io/k8s-skaffold/skaffold + - imageName: gcr.io/k8s-skaffold/skaffold docker: dockerfile: deploy/skaffold/Dockerfile deploy: From a3a9c6bd5bd037ac2ec74443716c3159a3e0ba63 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 8 Oct 2018 10:05:46 -0400 Subject: [PATCH 4/5] lints --- pkg/skaffold/schema/v1alpha3/upgrade_test.go | 23 +++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/pkg/skaffold/schema/v1alpha3/upgrade_test.go b/pkg/skaffold/schema/v1alpha3/upgrade_test.go index ab53cd77e8d..a3e522fb05d 100644 --- a/pkg/skaffold/schema/v1alpha3/upgrade_test.go +++ b/pkg/skaffold/schema/v1alpha3/upgrade_test.go @@ -1,7 +1,24 @@ +/* +Copyright 2018 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package v1alpha3 import ( "testing" + v1alpha4 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) @@ -37,21 +54,21 @@ profiles: if upgradedPipeline.Build.LocalBuild == nil { t.Errorf("expected build.local to be not nil") } - if upgradedPipeline.Build.LocalBuild.Push != nil && *upgradedPipeline.Build.LocalBuild.Push != true { + if upgradedPipeline.Build.LocalBuild.Push != nil && !*upgradedPipeline.Build.LocalBuild.Push { t.Errorf("expected build.local.push to be true but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) } if upgradedPipeline.Profiles[0].Build.LocalBuild == nil { t.Errorf("expected profiles[0].build.local to be not nil") } - if upgradedPipeline.Profiles[0].Build.LocalBuild.Push != nil && *upgradedPipeline.Profiles[0].Build.LocalBuild.Push != false { + if upgradedPipeline.Profiles[0].Build.LocalBuild.Push != nil && *upgradedPipeline.Profiles[0].Build.LocalBuild.Push { t.Errorf("expected profiles[0].build.local.push to be false but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) } if upgradedPipeline.Profiles[1].Build.LocalBuild == nil { t.Errorf("expected profiles[1].build.local to be not nil") } - if upgradedPipeline.Profiles[1].Build.LocalBuild.Push != nil && *upgradedPipeline.Profiles[1].Build.LocalBuild.Push != true { + if upgradedPipeline.Profiles[1].Build.LocalBuild.Push != nil && !*upgradedPipeline.Profiles[1].Build.LocalBuild.Push { t.Errorf("expected profiles[1].build.local.push to be true but it was: %v", *upgradedPipeline.Build.LocalBuild.Push) } } From 7df5967f21109ae41b570470959ff20bfeb5854e Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 8 Oct 2018 11:37:23 -0400 Subject: [PATCH 5/5] dockerfile -> dockerfilePath for main skaffold.yaml --- skaffold.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skaffold.yaml b/skaffold.yaml index a1a66d0522a..0376c15ff09 100644 --- a/skaffold.yaml +++ b/skaffold.yaml @@ -4,7 +4,7 @@ build: artifacts: - imageName: gcr.io/k8s-skaffold/skaffold docker: - dockerfile: deploy/skaffold/Dockerfile + dockerfilePath: deploy/skaffold/Dockerfile deploy: kubectl: manifests: