From d9ba197e6d1c6dbd9f259a2011ca3bf5e52ce902 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 18 Apr 2019 14:31:11 -0700 Subject: [PATCH 1/4] Remove `PreBuiltImageBuilder` and make it a function instead. For #922, i am making changes to `skaffold deploy` to not run `skaffold build`. `--images` flag is only used `deploy` flow. The way it works is when `--images` flag is present, runner creates a `PreBuiltImageBuilder`. All it does is, parses the images and converts them to build.Artifact. If during deploy, an image in the list was also built, then it uses the built image tag vs the tag present on the commmand line flag. In this change, we remove creating a PreBuiltImageBuilder in the runner. (Remember this flag is only available for deploy and hence other command won't get affected) The `--images` will now be converted to build.Artifact in the runner.Deploy flow. --- examples/kustomize/patch.yaml | 2 + integration/deploy_test.go | 3 +- integration/examples/kustomize/patch.yaml | 2 + integration/examples/kustomize/t.t | 1 + pkg/skaffold/build/prebuilt.go | 95 ------------------- pkg/skaffold/runner/prebuilt.go | 38 ++++++++ .../{build => runner}/prebuilt_test.go | 59 ++---------- pkg/skaffold/runner/runner.go | 15 ++- 8 files changed, 64 insertions(+), 151 deletions(-) create mode 100644 integration/examples/kustomize/t.t delete mode 100644 pkg/skaffold/build/prebuilt.go create mode 100644 pkg/skaffold/runner/prebuilt.go rename pkg/skaffold/{build => runner}/prebuilt_test.go (51%) diff --git a/examples/kustomize/patch.yaml b/examples/kustomize/patch.yaml index 0583d7a3318..de4b102edde 100644 --- a/examples/kustomize/patch.yaml +++ b/examples/kustomize/patch.yaml @@ -11,3 +11,5 @@ spec: command: - sleep - "3600" + - name: kustomize-test1 + image: index.docker.io/library/nginx diff --git a/integration/deploy_test.go b/integration/deploy_test.go index 4c4b412b0e7..f18bc45afcd 100644 --- a/integration/deploy_test.go +++ b/integration/deploy_test.go @@ -31,10 +31,11 @@ func TestDeploy(t *testing.T) { ns, client, deleteNs := SetupNamespace(t) defer deleteNs() - skaffold.Deploy("--images", "index.docker.io/library/busybox:1").InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t) + skaffold.Deploy("--images", "index.docker.io/library/busybox:1", "--images", "index.docker.io/library/nginx:2").InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t) dep := client.GetDeployment("kustomize-test") testutil.CheckDeepEqual(t, "index.docker.io/library/busybox:1", dep.Spec.Template.Spec.Containers[0].Image) + testutil.CheckDeepEqual(t, "index.docker.io/library/nginx:2", dep.Spec.Template.Spec.Containers[1].Image) skaffold.Delete().InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t) } diff --git a/integration/examples/kustomize/patch.yaml b/integration/examples/kustomize/patch.yaml index 0583d7a3318..de4b102edde 100644 --- a/integration/examples/kustomize/patch.yaml +++ b/integration/examples/kustomize/patch.yaml @@ -11,3 +11,5 @@ spec: command: - sleep - "3600" + - name: kustomize-test1 + image: index.docker.io/library/nginx diff --git a/integration/examples/kustomize/t.t b/integration/examples/kustomize/t.t new file mode 100644 index 00000000000..2ce5ef82f7e --- /dev/null +++ b/integration/examples/kustomize/t.t @@ -0,0 +1 @@ +{[]} \ No newline at end of file diff --git a/pkg/skaffold/build/prebuilt.go b/pkg/skaffold/build/prebuilt.go deleted file mode 100644 index 9bdbbf4843a..00000000000 --- a/pkg/skaffold/build/prebuilt.go +++ /dev/null @@ -1,95 +0,0 @@ -/* -Copyright 2019 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 build - -import ( - "context" - "io" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" - runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/context" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" - "github.com/pkg/errors" -) - -// NewPreBuiltImagesBuilder returns an new instance a Builder that assumes images are -// already built with given fully qualified names. -func NewPreBuiltImagesBuilder(runCtx *runcontext.RunContext) Builder { - return &prebuiltImagesBuilder{ - images: runCtx.Opts.PreBuiltImages, - } -} - -type prebuiltImagesBuilder struct { - images []string -} - -func (b *prebuiltImagesBuilder) Prune(_ context.Context, _ io.Writer) error { - // noop - return nil -} - -// Labels are labels applied to deployed resources. -func (b *prebuiltImagesBuilder) Labels() map[string]string { - return map[string]string{ - constants.Labels.Builder: "pre-built", - } -} - -func (b *prebuiltImagesBuilder) Build(ctx context.Context, out io.Writer, _ tag.ImageTags, artifacts []*latest.Artifact) ([]Artifact, error) { - tags := make(map[string]string) - - for _, tag := range b.images { - parsed, err := docker.ParseReference(tag) - if err != nil { - return nil, err - } - - tags[parsed.BaseName] = tag - } - - var builds []Artifact - - for _, artifact := range artifacts { - tag, present := tags[artifact.ImageName] - if !present { - return nil, errors.Errorf("unable to find image tag for %s", artifact.ImageName) - } - delete(tags, artifact.ImageName) - - builds = append(builds, Artifact{ - ImageName: artifact.ImageName, - Tag: tag, - }) - } - - for image, tag := range tags { - builds = append(builds, Artifact{ - ImageName: image, - Tag: tag, - }) - } - - return builds, nil -} - -// DependenciesForArtifact returns nil since a prebuilt image should have no dependencies -func (b *prebuiltImagesBuilder) DependenciesForArtifact(ctx context.Context, artifact *latest.Artifact) ([]string, error) { - return nil, nil -} diff --git a/pkg/skaffold/runner/prebuilt.go b/pkg/skaffold/runner/prebuilt.go new file mode 100644 index 00000000000..75fe20954b2 --- /dev/null +++ b/pkg/skaffold/runner/prebuilt.go @@ -0,0 +1,38 @@ +/* +Copyright 2019 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 runner + +import ( + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" +) + +func convertImagesToArtifact(images []string) ([]build.Artifact, error) { + var artifacts []build.Artifact + + for _, tag := range images { + parsed, err := docker.ParseReference(tag) + if err != nil { + return nil, err + } + artifacts = append(artifacts, build.Artifact{ + ImageName: parsed.BaseName, + Tag: tag, + }) + } + return artifacts, nil +} diff --git a/pkg/skaffold/build/prebuilt_test.go b/pkg/skaffold/runner/prebuilt_test.go similarity index 51% rename from pkg/skaffold/build/prebuilt_test.go rename to pkg/skaffold/runner/prebuilt_test.go index 7fc49b58d7d..147022498e5 100644 --- a/pkg/skaffold/build/prebuilt_test.go +++ b/pkg/skaffold/runner/prebuilt_test.go @@ -14,16 +14,12 @@ See the License for the specific language governing permissions and limitations under the License. */ -package build +package runner import ( - "context" - "io/ioutil" "testing" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" - runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/context" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -31,8 +27,7 @@ func TestPreBuiltImagesBuilder(t *testing.T) { var tests = []struct { description string images []string - artifacts []*latest.Artifact - expected []Artifact + expected []build.Artifact shouldErr bool }{ { @@ -41,41 +36,11 @@ func TestPreBuiltImagesBuilder(t *testing.T) { "skaffold/image1:tag1", "skaffold/image2:tag2", }, - artifacts: []*latest.Artifact{ - {ImageName: "skaffold/image1"}, - {ImageName: "skaffold/image2"}, - }, - expected: []Artifact{ - {ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"}, - {ImageName: "skaffold/image2", Tag: "skaffold/image2:tag2"}, - }, - }, - { - description: "images in reverse order", - images: []string{ - "skaffold/image2:tag2", - "skaffold/image1:tag1", - }, - artifacts: []*latest.Artifact{ - {ImageName: "skaffold/image1"}, - {ImageName: "skaffold/image2"}, - }, - expected: []Artifact{ + expected: []build.Artifact{ {ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"}, {ImageName: "skaffold/image2", Tag: "skaffold/image2:tag2"}, }, }, - { - description: "missing image", - images: []string{ - "skaffold/image1:tag1", - }, - artifacts: []*latest.Artifact{ - {ImageName: "skaffold/image1"}, - {ImageName: "skaffold/image2"}, - }, - shouldErr: true, - }, { // Should we support that? It is used in kustomize example. description: "additional image", @@ -83,25 +48,15 @@ func TestPreBuiltImagesBuilder(t *testing.T) { "busybox:1", "skaffold/image1:tag1", }, - artifacts: []*latest.Artifact{ - {ImageName: "skaffold/image1"}, - }, - expected: []Artifact{ - {ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"}, + expected: []build.Artifact{ {ImageName: "busybox", Tag: "busybox:1"}, + {ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"}, }, }, } for _, test := range tests { t.Run(test.description, func(t *testing.T) { - builder := NewPreBuiltImagesBuilder(&runcontext.RunContext{ - Opts: &config.SkaffoldOptions{ - PreBuiltImages: test.images, - }, - }) - - bRes, err := builder.Build(context.Background(), ioutil.Discard, nil, test.artifacts) - + bRes, err := convertImagesToArtifact(test.images) testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, bRes) }) } diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index e1faec2dd29..dfa03cdc99f 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -128,9 +128,9 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldConfig) (*Sk func getBuilder(runCtx *runcontext.RunContext) (build.Builder, error) { switch { - case len(runCtx.Opts.PreBuiltImages) > 0: - logrus.Debugln("Using pre-built images") - return build.NewPreBuiltImagesBuilder(runCtx), nil + case runCtx.Plugin: + logrus.Debugln("Using builder plugins") + return plugin.NewPluginBuilder(runCtx) case runCtx.Cfg.Build.LocalBuild != nil: logrus.Debugln("Using builder: local") @@ -331,6 +331,15 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa // Deploy deploys the given artifacts func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) error { + if len(r.runCtx.Opts.PreBuiltImages) > 0 { + logrus.Debugln("Deploy using pre-built images") + pBuilts, err := convertImagesToArtifact(r.runCtx.Opts.PreBuiltImages) + if err != nil { + return errors.Wrap(err, "deploy failed") + } + // Merge current build artifacts with previously built + artifacts = build.MergeWithPreviousBuilds(artifacts, pBuilts) + } err := r.Deployer.Deploy(ctx, out, artifacts, r.labellers) r.hasDeployed = true return err From be936d3f5295fb2c64fd3472092f07b03c20e987 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 18 Apr 2019 15:12:54 -0700 Subject: [PATCH 2/4] delete accidently checked in file --- integration/examples/kustomize/t.t | 1 - 1 file changed, 1 deletion(-) delete mode 100644 integration/examples/kustomize/t.t diff --git a/integration/examples/kustomize/t.t b/integration/examples/kustomize/t.t deleted file mode 100644 index 2ce5ef82f7e..00000000000 --- a/integration/examples/kustomize/t.t +++ /dev/null @@ -1 +0,0 @@ -{[]} \ No newline at end of file From 20e2afc19f0ca5d4ce6b79047b2b9d1616f5dd78 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Mon, 22 Apr 2019 10:23:36 -0700 Subject: [PATCH 3/4] accidental wrong rebase --- pkg/skaffold/runner/runner.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index dfa03cdc99f..a84db95b2fe 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -128,10 +128,6 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldConfig) (*Sk func getBuilder(runCtx *runcontext.RunContext) (build.Builder, error) { switch { - case runCtx.Plugin: - logrus.Debugln("Using builder plugins") - return plugin.NewPluginBuilder(runCtx) - case runCtx.Cfg.Build.LocalBuild != nil: logrus.Debugln("Using builder: local") return local.NewBuilder(runCtx) From a3905d1595c9e5881797f8b8d2f91e93b95ac111 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Tue, 23 Apr 2019 09:48:10 -0700 Subject: [PATCH 4/4] Add better commit msg --- pkg/skaffold/runner/runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index a84db95b2fe..1e0473cef90 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -333,7 +333,7 @@ func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts [] if err != nil { return errors.Wrap(err, "deploy failed") } - // Merge current build artifacts with previously built + // Merge current build artifacts with previously built passed in --images. artifacts = build.MergeWithPreviousBuilds(artifacts, pBuilts) } err := r.Deployer.Deploy(ctx, out, artifacts, r.labellers)