From 4c3eae6a6fe56607e712676deb174b0629fb0ee5 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 18 Apr 2019 14:31:11 -0700 Subject: [PATCH] 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 | 13 ++- 8 files changed, 61 insertions(+), 152 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 15cbe6e7be8..cee5c446702 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -133,10 +133,6 @@ func getBuilder(runCtx *runcontext.RunContext) (build.Builder, error) { logrus.Debugln("Using builder plugins") return plugin.NewPluginBuilder(runCtx) - case len(runCtx.Opts.PreBuiltImages) > 0: - logrus.Debugln("Using pre-built images") - return build.NewPreBuiltImagesBuilder(runCtx), nil - case runCtx.Cfg.Build.LocalBuild != nil: logrus.Debugln("Using builder: local") return local.NewBuilder(runCtx) @@ -336,6 +332,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