From ed428eb94a892669e7b84bf76e2de17a5888ccb2 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 10:59:10 -0500 Subject: [PATCH 01/13] FakeApi produces more realistic imageID and digest Signed-off-by: David Gageot --- pkg/skaffold/docker/image_test.go | 4 ++-- testutil/fake_image_api.go | 15 ++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/pkg/skaffold/docker/image_test.go b/pkg/skaffold/docker/image_test.go index 3f872bd2fda..a58d8dd6b4b 100644 --- a/pkg/skaffold/docker/image_test.go +++ b/pkg/skaffold/docker/image_test.go @@ -49,10 +49,10 @@ func TestPush(t *testing.T) { imageName: "gcr.io/scratchman", api: testutil.FakeAPIClient{ TagToImageID: map[string]string{ - "gcr.io/scratchman": "sha256:abcab", + "gcr.io/scratchman": "sha256:imageIDabcab", }, }, - expectedDigest: "sha256:abcab", + expectedDigest: "sha256:7368613235363a696d61676549446162636162e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", }, { description: "stream error", diff --git a/testutil/fake_image_api.go b/testutil/fake_image_api.go index 77b5bea49ab..3c8ad4310e7 100644 --- a/testutil/fake_image_api.go +++ b/testutil/fake_image_api.go @@ -18,7 +18,7 @@ package testutil import ( "context" - "crypto/rand" + "crypto/sha256" "fmt" "io" "io/ioutil" @@ -39,6 +39,8 @@ type FakeAPIClient struct { ErrImagePush bool ErrImagePull bool ErrStream bool + + nextImageID int } type errReader struct{} @@ -62,7 +64,8 @@ func (f *FakeAPIClient) ImageBuild(_ context.Context, _ io.Reader, options types f.TagToImageID = make(map[string]string) } - imageID := "sha256:" + randomID() + f.nextImageID++ + imageID := fmt.Sprintf("sha256:%d", f.nextImageID) f.TagToImageID[imageID] = imageID for _, tag := range options.Tags { @@ -77,12 +80,6 @@ func (f *FakeAPIClient) ImageBuild(_ context.Context, _ io.Reader, options types }, nil } -func randomID() string { - b := make([]byte, 16) - rand.Read(b) - return fmt.Sprintf("%x", b) -} - func (f *FakeAPIClient) ImageInspectWithRaw(_ context.Context, ref string) (types.ImageInspect, []byte, error) { if f.ErrImageInspect { return types.ImageInspect{}, nil, fmt.Errorf("") @@ -116,7 +113,7 @@ func (f *FakeAPIClient) ImagePush(_ context.Context, ref string, _ types.ImagePu return nil, fmt.Errorf("") } - digest := f.TagToImageID[ref] + digest := fmt.Sprintf("sha256:%x", sha256.New().Sum([]byte(f.TagToImageID[ref]))) return f.body(digest), nil } From fc25ff530f9c267555b547a836cf3a928cece56c Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 09:13:55 -0500 Subject: [PATCH 02/13] Remove small optimization Signed-off-by: David Gageot --- pkg/skaffold/build/local/local.go | 9 --------- pkg/skaffold/build/local/types.go | 2 -- 2 files changed, 11 deletions(-) diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 268bf8d2344..afe92594209 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -47,13 +47,6 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "build artifact") } - if b.alreadyTagged == nil { - b.alreadyTagged = make(map[string]string) - } - if tag, present := b.alreadyTagged[digest]; present { - return tag, nil - } - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, Digest: digest, @@ -66,8 +59,6 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "tagging") } - b.alreadyTagged[digest] = tag - return tag, nil } diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go index 0880b8d64dd..71eb2e7fe49 100644 --- a/pkg/skaffold/build/local/types.go +++ b/pkg/skaffold/build/local/types.go @@ -37,8 +37,6 @@ type Builder struct { pushImages bool skipTests bool kubeContext string - - alreadyTagged map[string]string } // NewBuilder returns an new instance of a local Builder. From 8ae0afee35b2162770445ea5d6a85787f141218a Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 09:22:46 -0500 Subject: [PATCH 03/13] Always use the digest/imageID to deploy Actually, we use: + imageID when the image is built locally + tag@sha256:abcabc when the image was pushed We assume here that taggers never append the digest to the tag themselves. (Which is not true) Signed-off-by: David Gageot --- pkg/skaffold/build/gcb/cloud_build.go | 2 +- pkg/skaffold/build/kaniko/kaniko.go | 2 +- pkg/skaffold/build/local/local.go | 7 ++++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 529de73c22b..5cea0d3531e 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -167,7 +167,7 @@ watch: return "", errors.Wrap(err, "tagging image") } - return newTag, nil + return newTag + "@" + digest, nil } func getBuildID(op *cloudbuild.Operation) (string, error) { diff --git a/pkg/skaffold/build/kaniko/kaniko.go b/pkg/skaffold/build/kaniko/kaniko.go index ad62bdc31ac..e69acc2bfa8 100644 --- a/pkg/skaffold/build/kaniko/kaniko.go +++ b/pkg/skaffold/build/kaniko/kaniko.go @@ -69,5 +69,5 @@ func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, ta return "", errors.Wrap(err, "tagging image") } - return tag, nil + return tag + "@" + digest, nil } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index afe92594209..39bb6ed1214 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -59,7 +59,12 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "tagging") } - return tag, nil + if b.pushImages { + return tag + "@" + digest, nil + } else { + // Here, digest is the imageID + return digest, nil + } } func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact) (string, error) { From 942b092dc8bc2ae49e786b13fa63a47b300684db Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 09:24:12 -0500 Subject: [PATCH 04/13] =?UTF-8?q?Let=E2=80=99s=20assume=20taggers=20don?= =?UTF-8?q?=E2=80=99t=20need=20the=20digest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Gageot --- pkg/skaffold/build/gcb/cloud_build.go | 1 - pkg/skaffold/build/kaniko/kaniko.go | 1 - pkg/skaffold/build/local/local.go | 1 - pkg/skaffold/build/sequence_test.go | 2 +- 4 files changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 5cea0d3531e..9dce5118aa4 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -157,7 +157,6 @@ watch: newTag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, - Digest: digest, }) if err != nil { return "", errors.Wrap(err, "generating tag") diff --git a/pkg/skaffold/build/kaniko/kaniko.go b/pkg/skaffold/build/kaniko/kaniko.go index e69acc2bfa8..8ce09ca60e5 100644 --- a/pkg/skaffold/build/kaniko/kaniko.go +++ b/pkg/skaffold/build/kaniko/kaniko.go @@ -59,7 +59,6 @@ func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, ta tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, - Digest: digest, }) if err != nil { return "", errors.Wrap(err, "generating tag") diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 39bb6ed1214..e648278591c 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -49,7 +49,6 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, - Digest: digest, }) if err != nil { return "", errors.Wrap(err, "generating tag") diff --git a/pkg/skaffold/build/sequence_test.go b/pkg/skaffold/build/sequence_test.go index 0fc59f10c10..4e324615236 100644 --- a/pkg/skaffold/build/sequence_test.go +++ b/pkg/skaffold/build/sequence_test.go @@ -33,7 +33,7 @@ type MockTagger struct { Err error } -func (f *MockTagger) GenerateFullyQualifiedImageName(workingDir string, tagOpts tag.Options) (string, error) { +func (f *MockTagger) GenerateFullyQualifiedImageName(workingDir string, imageName string) (string, error) { return f.Out, f.Err } From b2de9ddc917e7e625d2eb56c78925abd52679d84 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 09:25:00 -0500 Subject: [PATCH 05/13] Compute the tag before the build Signed-off-by: David Gageot --- pkg/skaffold/build/gcb/cloud_build.go | 14 +++++++------- pkg/skaffold/build/kaniko/kaniko.go | 14 +++++++------- pkg/skaffold/build/local/local.go | 10 +++++----- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 9dce5118aa4..fc42896e99d 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -49,6 +49,13 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { + newTag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ + ImageName: artifact.ImageName, + }) + if err != nil { + return "", errors.Wrap(err, "generating tag") + } + client, err := google.DefaultClient(ctx, cloudbuild.CloudPlatformScope) if err != nil { return "", errors.Wrap(err, "getting google client") @@ -155,13 +162,6 @@ watch: builtTag := fmt.Sprintf("%s@%s", artifact.ImageName, digest) logrus.Infof("Image built at %s", builtTag) - newTag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ - ImageName: artifact.ImageName, - }) - if err != nil { - return "", errors.Wrap(err, "generating tag") - } - if err := docker.AddTag(builtTag, newTag); err != nil { return "", errors.Wrap(err, "tagging image") } diff --git a/pkg/skaffold/build/kaniko/kaniko.go b/pkg/skaffold/build/kaniko/kaniko.go index 8ce09ca60e5..ecb34bb1482 100644 --- a/pkg/skaffold/build/kaniko/kaniko.go +++ b/pkg/skaffold/build/kaniko/kaniko.go @@ -47,6 +47,13 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { + tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ + ImageName: artifact.ImageName, + }) + if err != nil { + return "", errors.Wrap(err, "generating tag") + } + initialTag, err := b.run(ctx, out, artifact) if err != nil { return "", errors.Wrapf(err, "kaniko build for [%s]", artifact.ImageName) @@ -57,13 +64,6 @@ func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, ta return "", errors.Wrap(err, "getting digest") } - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ - ImageName: artifact.ImageName, - }) - if err != nil { - return "", errors.Wrap(err, "generating tag") - } - if err := docker.AddTag(initialTag, tag); err != nil { return "", errors.Wrap(err, "tagging image") } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index e648278591c..3183aa28d5e 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -42,11 +42,6 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { - digest, err := b.runBuildForArtifact(ctx, out, artifact) - if err != nil { - return "", errors.Wrap(err, "build artifact") - } - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, }) @@ -54,6 +49,11 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "generating tag") } + digest, err := b.runBuildForArtifact(ctx, out, artifact) + if err != nil { + return "", errors.Wrap(err, "build artifact") + } + if err := b.retagAndPush(ctx, out, digest, tag, artifact); err != nil { return "", errors.Wrap(err, "tagging") } From 30e6794188d51eb1ca2cdbdb9b70cbfc5cabd538 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 09:34:07 -0500 Subject: [PATCH 06/13] Remove retagging Pass the final tag to the builders and let them build and push that tag directly. --- pkg/skaffold/build/gcb/cloud_build.go | 13 +--- pkg/skaffold/build/gcb/desc.go | 14 ++--- pkg/skaffold/build/gcb/desc_test.go | 2 +- pkg/skaffold/build/gcb/docker.go | 4 +- pkg/skaffold/build/gcb/docker_test.go | 7 +-- pkg/skaffold/build/gcb/jib.go | 8 +-- pkg/skaffold/build/gcb/jib_test.go | 4 +- pkg/skaffold/build/kaniko/kaniko.go | 12 +--- pkg/skaffold/build/kaniko/run.go | 11 ++-- pkg/skaffold/build/local/bazel.go | 25 ++++---- pkg/skaffold/build/local/docker.go | 10 ++- pkg/skaffold/build/local/jib.go | 43 ------------- pkg/skaffold/build/local/jib_gradle.go | 24 +++---- pkg/skaffold/build/local/jib_maven.go | 28 ++++----- .../local/{jib_test.go => jib_maven_test.go} | 19 ------ pkg/skaffold/build/local/local.go | 58 ++++++----------- pkg/skaffold/build/local/local_test.go | 63 +++++++------------ 17 files changed, 108 insertions(+), 237 deletions(-) delete mode 100644 pkg/skaffold/build/local/jib.go rename pkg/skaffold/build/local/{jib_test.go => jib_maven_test.go} (78%) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index fc42896e99d..7ffa9b6406c 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -29,7 +29,6 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/gcp" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/sources" @@ -49,7 +48,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { - newTag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ + tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ ImageName: artifact.ImageName, }) if err != nil { @@ -93,7 +92,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer return "", errors.Wrap(err, "checking bucket is in correct project") } - desc, err := b.buildDescription(artifact, cbBucket, buildObject) + desc, err := b.buildDescription(artifact, tag, cbBucket, buildObject) if err != nil { return "", errors.Wrap(err, "could not create build description") } @@ -159,14 +158,8 @@ watch: return "", errors.Wrap(err, "cleaning up source tar after build") } logrus.Infof("Deleted object %s", buildObject) - builtTag := fmt.Sprintf("%s@%s", artifact.ImageName, digest) - logrus.Infof("Image built at %s", builtTag) - if err := docker.AddTag(builtTag, newTag); err != nil { - return "", errors.Wrap(err, "tagging image") - } - - return newTag + "@" + digest, nil + return tag + "@" + digest, nil } func getBuildID(op *cloudbuild.Operation) (string, error) { diff --git a/pkg/skaffold/build/gcb/desc.go b/pkg/skaffold/build/gcb/desc.go index 7698f940970..d2559d93ddb 100644 --- a/pkg/skaffold/build/gcb/desc.go +++ b/pkg/skaffold/build/gcb/desc.go @@ -24,8 +24,8 @@ import ( cloudbuild "google.golang.org/api/cloudbuild/v1" ) -func (b *Builder) buildDescription(artifact *latest.Artifact, bucket, object string) (*cloudbuild.Build, error) { - steps, err := b.buildSteps(artifact) +func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, object string) (*cloudbuild.Build, error) { + steps, err := b.buildSteps(artifact, tag) if err != nil { return nil, err } @@ -39,7 +39,7 @@ func (b *Builder) buildDescription(artifact *latest.Artifact, bucket, object str }, }, Steps: steps, - Images: []string{artifact.ImageName}, + Images: []string{tag}, Options: &cloudbuild.BuildOptions{ DiskSizeGb: b.DiskSizeGb, MachineType: b.MachineType, @@ -48,19 +48,19 @@ func (b *Builder) buildDescription(artifact *latest.Artifact, bucket, object str }, nil } -func (b *Builder) buildSteps(artifact *latest.Artifact) ([]*cloudbuild.BuildStep, error) { +func (b *Builder) buildSteps(artifact *latest.Artifact, tag string) ([]*cloudbuild.BuildStep, error) { switch { case artifact.DockerArtifact != nil: - return b.dockerBuildSteps(artifact.ImageName, artifact.DockerArtifact), nil + return b.dockerBuildSteps(artifact.DockerArtifact, tag), nil case artifact.BazelArtifact != nil: return nil, errors.New("skaffold can't build a bazel artifact with Google Cloud Build") case artifact.JibMavenArtifact != nil: - return b.jibMavenBuildSteps(artifact.ImageName, artifact.JibMavenArtifact), nil + return b.jibMavenBuildSteps(artifact.JibMavenArtifact, tag), nil case artifact.JibGradleArtifact != nil: - return b.jibGradleBuildSteps(artifact.ImageName, artifact.JibGradleArtifact), nil + return b.jibGradleBuildSteps(artifact.JibGradleArtifact, tag), nil default: return nil, fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType) diff --git a/pkg/skaffold/build/gcb/desc_test.go b/pkg/skaffold/build/gcb/desc_test.go index f62efc8f0a5..11df1ab5e02 100644 --- a/pkg/skaffold/build/gcb/desc_test.go +++ b/pkg/skaffold/build/gcb/desc_test.go @@ -33,7 +33,7 @@ func TestBuildBazelDescriptionFail(t *testing.T) { builder := Builder{ GoogleCloudBuild: &latest.GoogleCloudBuild{}, } - _, err := builder.buildDescription(artifact, "bucket", "object") + _, err := builder.buildDescription(artifact, "tag", "bucket", "object") testutil.CheckError(t, true, err) } diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go index ab75015ec4b..c7d8d960ced 100644 --- a/pkg/skaffold/build/gcb/docker.go +++ b/pkg/skaffold/build/gcb/docker.go @@ -22,7 +22,7 @@ import ( cloudbuild "google.golang.org/api/cloudbuild/v1" ) -func (b *Builder) dockerBuildSteps(imageName string, artifact *latest.DockerArtifact) []*cloudbuild.BuildStep { +func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string) []*cloudbuild.BuildStep { var steps []*cloudbuild.BuildStep for _, cacheFrom := range artifact.CacheFrom { @@ -32,7 +32,7 @@ func (b *Builder) dockerBuildSteps(imageName string, artifact *latest.DockerArti }) } - args := append([]string{"build", "--tag", imageName, "-f", artifact.DockerfilePath}) + args := append([]string{"build", "--tag", tag, "-f", artifact.DockerfilePath}) args = append(args, docker.GetBuildArgs(artifact)...) args = append(args, ".") diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index 8f6c2e3675a..ed13ae0b29e 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -27,7 +27,6 @@ import ( func TestDockerBuildDescription(t *testing.T) { artifact := &latest.Artifact{ - ImageName: "nginx", ArtifactType: latest.ArtifactType{ DockerArtifact: &latest.DockerArtifact{ DockerfilePath: "Dockerfile", @@ -47,7 +46,7 @@ func TestDockerBuildDescription(t *testing.T) { Timeout: "10m", }, } - desc, err := builder.buildDescription(artifact, "bucket", "object") + desc, err := builder.buildDescription(artifact, "nginx", "bucket", "object") expected := cloudbuild.Build{ LogsBucket: "bucket", @@ -61,7 +60,7 @@ func TestDockerBuildDescription(t *testing.T) { Name: "docker/docker", Args: []string{"build", "--tag", "nginx", "-f", "Dockerfile", "--build-arg", "arg1=value1", "--build-arg", "arg2", "."}, }}, - Images: []string{artifact.ImageName}, + Images: []string{"nginx"}, Options: &cloudbuild.BuildOptions{ DiskSizeGb: 100, MachineType: "n1-standard-1", @@ -83,7 +82,7 @@ func TestPullCacheFrom(t *testing.T) { DockerImage: "docker/docker", }, } - steps := builder.dockerBuildSteps("nginx2", artifact) + steps := builder.dockerBuildSteps(artifact, "nginx2") expected := []*cloudbuild.BuildStep{{ Name: "docker/docker", diff --git a/pkg/skaffold/build/gcb/jib.go b/pkg/skaffold/build/gcb/jib.go index d4d977ec25f..9a1dc218395 100644 --- a/pkg/skaffold/build/gcb/jib.go +++ b/pkg/skaffold/build/gcb/jib.go @@ -23,16 +23,16 @@ import ( ) // TODO(dgageot): check that `package` is bound to `jib:build` -func (b *Builder) jibMavenBuildSteps(imageName string, artifact *latest.JibMavenArtifact) []*cloudbuild.BuildStep { +func (b *Builder) jibMavenBuildSteps(artifact *latest.JibMavenArtifact, tag string) []*cloudbuild.BuildStep { return []*cloudbuild.BuildStep{{ Name: b.MavenImage, - Args: jib.GenerateMavenArgs("dockerBuild", imageName, artifact, b.skipTests), + Args: jib.GenerateMavenArgs("dockerBuild", tag, artifact, b.skipTests), }} } -func (b *Builder) jibGradleBuildSteps(imageName string, artifact *latest.JibGradleArtifact) []*cloudbuild.BuildStep { +func (b *Builder) jibGradleBuildSteps(artifact *latest.JibGradleArtifact, tag string) []*cloudbuild.BuildStep { return []*cloudbuild.BuildStep{{ Name: b.GradleImage, - Args: jib.GenerateGradleArgs("jibDockerBuild", imageName, artifact, b.skipTests), + Args: jib.GenerateGradleArgs("jibDockerBuild", tag, artifact, b.skipTests), }} } diff --git a/pkg/skaffold/build/gcb/jib_test.go b/pkg/skaffold/build/gcb/jib_test.go index 9f99e747801..e0e58c313b4 100644 --- a/pkg/skaffold/build/gcb/jib_test.go +++ b/pkg/skaffold/build/gcb/jib_test.go @@ -41,7 +41,7 @@ func TestJibMavenBuildSteps(t *testing.T) { }, skipTests: tt.skipTests, } - steps := builder.jibMavenBuildSteps("img", artifact) + steps := builder.jibMavenBuildSteps(artifact, "img") expected := []*cloudbuild.BuildStep{{ Name: "maven:3.6.0", @@ -69,7 +69,7 @@ func TestJibGradleBuildSteps(t *testing.T) { }, skipTests: tt.skipTests, } - steps := builder.jibGradleBuildSteps("img", artifact) + steps := builder.jibGradleBuildSteps(artifact, "img") expected := []*cloudbuild.BuildStep{{ Name: "gradle:5.1.1", diff --git a/pkg/skaffold/build/kaniko/kaniko.go b/pkg/skaffold/build/kaniko/kaniko.go index ecb34bb1482..db294aa7e65 100644 --- a/pkg/skaffold/build/kaniko/kaniko.go +++ b/pkg/skaffold/build/kaniko/kaniko.go @@ -22,7 +22,6 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/pkg/errors" ) @@ -54,19 +53,10 @@ func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, ta return "", errors.Wrap(err, "generating tag") } - initialTag, err := b.run(ctx, out, artifact) + digest, err := b.run(ctx, out, artifact, tag) if err != nil { return "", errors.Wrapf(err, "kaniko build for [%s]", artifact.ImageName) } - digest, err := docker.RemoteDigest(initialTag) - if err != nil { - return "", errors.Wrap(err, "getting digest") - } - - if err := docker.AddTag(initialTag, tag); err != nil { - return "", errors.Wrap(err, "tagging image") - } - return tag + "@" + digest, nil } diff --git a/pkg/skaffold/build/kaniko/run.go b/pkg/skaffold/build/kaniko/run.go index d6cf7364661..8b820524404 100644 --- a/pkg/skaffold/build/kaniko/run.go +++ b/pkg/skaffold/build/kaniko/run.go @@ -31,17 +31,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func (b *Builder) run(ctx context.Context, out io.Writer, artifact *latest.Artifact) (string, error) { +func (b *Builder) run(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { if artifact.DockerArtifact == nil { return "", errors.New("kaniko builder supports only Docker artifacts") } - initialTag := util.RandomID() - imageDst := fmt.Sprintf("%s:%s", artifact.ImageName, initialTag) - // Prepare context s := sources.Retrieve(b.KanikoBuild) - context, err := s.Setup(ctx, out, artifact, initialTag) + context, err := s.Setup(ctx, out, artifact, util.RandomID()) if err != nil { return "", errors.Wrap(err, "setting up build context") } @@ -51,7 +48,7 @@ func (b *Builder) run(ctx context.Context, out io.Writer, artifact *latest.Artif args := []string{ "--dockerfile", artifact.DockerArtifact.DockerfilePath, "--context", context, - "--destination", imageDst, + "--destination", tag, "-v", logLevel().String()} args = append(args, b.AdditionalFlags...) args = append(args, docker.GetBuildArgs(artifact.DockerArtifact)...) @@ -96,5 +93,5 @@ func (b *Builder) run(ctx context.Context, out io.Writer, artifact *latest.Artif waitForLogs() - return imageDst, nil + return docker.RemoteDigest(tag) } diff --git a/pkg/skaffold/build/local/bazel.go b/pkg/skaffold/build/local/bazel.go index 728247e1471..1f70f2a7697 100644 --- a/pkg/skaffold/build/local/bazel.go +++ b/pkg/skaffold/build/local/bazel.go @@ -36,10 +36,10 @@ import ( "github.com/pkg/errors" ) -func (b *Builder) buildBazel(ctx context.Context, out io.Writer, workspace string, a *latest.Artifact) (string, error) { +func (b *Builder) buildBazel(ctx context.Context, out io.Writer, workspace string, a *latest.BazelArtifact, tag string) (string, error) { args := []string{"build"} - args = append(args, a.BazelArtifact.BuildArgs...) - args = append(args, a.BazelArtifact.BuildTarget) + args = append(args, a.BuildArgs...) + args = append(args, a.BuildTarget) // FIXME: is it possible to apply b.skipTests? cmd := exec.CommandContext(ctx, "bazel", args...) @@ -50,20 +50,19 @@ func (b *Builder) buildBazel(ctx context.Context, out io.Writer, workspace strin return "", errors.Wrap(err, "running command") } - bazelBin, err := bazelBin(ctx, workspace, a.BazelArtifact) + bazelBin, err := bazelBin(ctx, workspace, a) if err != nil { return "", errors.Wrap(err, "getting path of bazel-bin") } - tarPath := filepath.Join(bazelBin, buildTarPath(a.BazelArtifact.BuildTarget)) + tarPath := filepath.Join(bazelBin, buildTarPath(a.BuildTarget)) if b.pushImages { - uniqueTag := a.ImageName + ":" + util.RandomID() - return pushImage(tarPath, uniqueTag) + return pushImage(tarPath, tag) } - ref := buildImageTag(a.BazelArtifact.BuildTarget) - return b.loadImage(ctx, out, tarPath, ref) + bazelTag := buildImageTag(a.BuildTarget) + return b.loadImage(ctx, out, tarPath, bazelTag, tag) } func pushImage(tarPath, tag string) (string, error) { @@ -89,18 +88,22 @@ func pushImage(tarPath, tag string) (string, error) { return docker.RemoteDigest(tag) } -func (b *Builder) loadImage(ctx context.Context, out io.Writer, tarPath string, ref string) (string, error) { +func (b *Builder) loadImage(ctx context.Context, out io.Writer, tarPath string, bazelTag string, tag string) (string, error) { imageTar, err := os.Open(tarPath) if err != nil { return "", errors.Wrap(err, "opening image tarball") } defer imageTar.Close() - imageID, err := b.localDocker.Load(ctx, out, imageTar, ref) + imageID, err := b.localDocker.Load(ctx, out, imageTar, bazelTag) if err != nil { return "", errors.Wrap(err, "loading image into docker daemon") } + if err := b.localDocker.Tag(ctx, imageID, tag); err != nil { + return "", errors.Wrap(err, "tagging the image") + } + return imageID, nil } diff --git a/pkg/skaffold/build/local/docker.go b/pkg/skaffold/build/local/docker.go index 5ac00a6d4f2..7a8b3a1704d 100644 --- a/pkg/skaffold/build/local/docker.go +++ b/pkg/skaffold/build/local/docker.go @@ -28,20 +28,18 @@ import ( "github.com/pkg/errors" ) -func (b *Builder) buildDocker(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact) (string, error) { +func (b *Builder) buildDocker(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, tag string) (string, error) { if err := b.pullCacheFromImages(ctx, out, a); err != nil { return "", errors.Wrap(err, "pulling cache-from images") } - initialTag := util.RandomID() - if b.cfg.UseDockerCLI || b.cfg.UseBuildkit { dockerfilePath, err := docker.NormalizeDockerfilePath(workspace, a.DockerfilePath) if err != nil { return "", errors.Wrap(err, "normalizing dockerfile path") } - args := []string{"build", workspace, "--file", dockerfilePath, "-t", initialTag} + args := []string{"build", workspace, "--file", dockerfilePath, "-t", tag} args = append(args, docker.GetBuildArgs(a)...) cmd := exec.CommandContext(ctx, "docker", args...) @@ -55,10 +53,10 @@ func (b *Builder) buildDocker(ctx context.Context, out io.Writer, workspace stri return "", errors.Wrap(err, "running build") } - return b.localDocker.ImageID(ctx, initialTag) + return b.localDocker.ImageID(ctx, tag) } - return b.localDocker.Build(ctx, out, workspace, a, initialTag) + return b.localDocker.Build(ctx, out, workspace, a, tag) } func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *latest.DockerArtifact) error { diff --git a/pkg/skaffold/build/local/jib.go b/pkg/skaffold/build/local/jib.go deleted file mode 100644 index f16d074cf6f..00000000000 --- a/pkg/skaffold/build/local/jib.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -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 local - -import ( - "crypto/sha1" - "encoding/hex" - "io" - "regexp" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" -) - -// jibBuildImageRef generates a valid image name for the workspace and project. -// The image name is always prefixed with `jib`. -func generateJibImageRef(workspace string, project string) string { - imageName := "jib" + workspace - if project != "" { - imageName += "_" + project - } - // if the workspace + project is a valid image name then use it - if regexp.MustCompile(constants.RepositoryComponentRegex).MatchString(imageName) { - return imageName - } - // otherwise use a hash for a deterministic name - hasher := sha1.New() - io.WriteString(hasher, imageName) - return "jib__" + hex.EncodeToString(hasher.Sum(nil)) -} diff --git a/pkg/skaffold/build/local/jib_gradle.go b/pkg/skaffold/build/local/jib_gradle.go index 599ccfecb1f..0f9784530b2 100644 --- a/pkg/skaffold/build/local/jib_gradle.go +++ b/pkg/skaffold/build/local/jib_gradle.go @@ -18,7 +18,6 @@ package local import ( "context" - "fmt" "io" "os" @@ -30,34 +29,29 @@ import ( "github.com/sirupsen/logrus" ) -func (b *Builder) buildJibGradle(ctx context.Context, out io.Writer, workspace string, artifact *latest.Artifact) (string, error) { +func (b *Builder) buildJibGradle(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibGradleArtifact, tag string) (string, error) { if b.pushImages { - return b.buildJibGradleToRegistry(ctx, out, workspace, artifact) + return b.buildJibGradleToRegistry(ctx, out, workspace, artifact, tag) } - return b.buildJibGradleToDocker(ctx, out, workspace, artifact.JibGradleArtifact) + return b.buildJibGradleToDocker(ctx, out, workspace, artifact, tag) } -func (b *Builder) buildJibGradleToDocker(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibGradleArtifact) (string, error) { - skaffoldImage := generateJibImageRef(workspace, artifact.Project) - args := jib.GenerateGradleArgs("jibDockerBuild", skaffoldImage, artifact, b.skipTests) - +func (b *Builder) buildJibGradleToDocker(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibGradleArtifact, tag string) (string, error) { + args := jib.GenerateGradleArgs("jibDockerBuild", tag, artifact, b.skipTests) if err := b.runGradleCommand(ctx, out, workspace, args); err != nil { return "", err } - return b.localDocker.ImageID(ctx, skaffoldImage) + return b.localDocker.ImageID(ctx, tag) } -func (b *Builder) buildJibGradleToRegistry(ctx context.Context, out io.Writer, workspace string, artifact *latest.Artifact) (string, error) { - initialTag := util.RandomID() - skaffoldImage := fmt.Sprintf("%s:%s", artifact.ImageName, initialTag) - args := jib.GenerateGradleArgs("jib", skaffoldImage, artifact.JibGradleArtifact, b.skipTests) - +func (b *Builder) buildJibGradleToRegistry(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibGradleArtifact, tag string) (string, error) { + args := jib.GenerateGradleArgs("jib", tag, artifact, b.skipTests) if err := b.runGradleCommand(ctx, out, workspace, args); err != nil { return "", err } - return docker.RemoteDigest(skaffoldImage) + return docker.RemoteDigest(tag) } func (b *Builder) runGradleCommand(ctx context.Context, out io.Writer, workspace string, args []string) error { diff --git a/pkg/skaffold/build/local/jib_maven.go b/pkg/skaffold/build/local/jib_maven.go index d5c866a35c4..783ac2ca565 100644 --- a/pkg/skaffold/build/local/jib_maven.go +++ b/pkg/skaffold/build/local/jib_maven.go @@ -18,7 +18,6 @@ package local import ( "context" - "fmt" "io" "os" @@ -30,14 +29,14 @@ import ( "github.com/sirupsen/logrus" ) -func (b *Builder) buildJibMaven(ctx context.Context, out io.Writer, workspace string, artifact *latest.Artifact) (string, error) { +func (b *Builder) buildJibMaven(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibMavenArtifact, tag string) (string, error) { if b.pushImages { - return b.buildJibMavenToRegistry(ctx, out, workspace, artifact) + return b.buildJibMavenToRegistry(ctx, out, workspace, artifact, tag) } - return b.buildJibMavenToDocker(ctx, out, workspace, artifact.JibMavenArtifact) + return b.buildJibMavenToDocker(ctx, out, workspace, artifact, tag) } -func (b *Builder) buildJibMavenToDocker(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibMavenArtifact) (string, error) { +func (b *Builder) buildJibMavenToDocker(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibMavenArtifact, tag string) (string, error) { // If this is a multi-module project, we require `package` be bound to jib:dockerBuild if artifact.Module != "" { if err := verifyJibPackageGoal(ctx, "dockerBuild", workspace, artifact); err != nil { @@ -45,33 +44,28 @@ func (b *Builder) buildJibMavenToDocker(ctx context.Context, out io.Writer, work } } - skaffoldImage := generateJibImageRef(workspace, artifact.Module) - args := jib.GenerateMavenArgs("dockerBuild", skaffoldImage, artifact, b.skipTests) - + args := jib.GenerateMavenArgs("dockerBuild", tag, artifact, b.skipTests) if err := b.runMavenCommand(ctx, out, workspace, args); err != nil { return "", err } - return b.localDocker.ImageID(ctx, skaffoldImage) + return b.localDocker.ImageID(ctx, tag) } -func (b *Builder) buildJibMavenToRegistry(ctx context.Context, out io.Writer, workspace string, artifact *latest.Artifact) (string, error) { +func (b *Builder) buildJibMavenToRegistry(ctx context.Context, out io.Writer, workspace string, artifact *latest.JibMavenArtifact, tag string) (string, error) { // If this is a multi-module project, we require `package` be bound to jib:build - if artifact.JibMavenArtifact.Module != "" { - if err := verifyJibPackageGoal(ctx, "build", workspace, artifact.JibMavenArtifact); err != nil { + if artifact.Module != "" { + if err := verifyJibPackageGoal(ctx, "build", workspace, artifact); err != nil { return "", err } } - initialTag := util.RandomID() - skaffoldImage := fmt.Sprintf("%s:%s", artifact.ImageName, initialTag) - args := jib.GenerateMavenArgs("build", skaffoldImage, artifact.JibMavenArtifact, b.skipTests) - + args := jib.GenerateMavenArgs("build", tag, artifact, b.skipTests) if err := b.runMavenCommand(ctx, out, workspace, args); err != nil { return "", err } - return docker.RemoteDigest(skaffoldImage) + return docker.RemoteDigest(tag) } // verifyJibPackageGoal verifies that the referenced module has `package` bound to a single jib goal. diff --git a/pkg/skaffold/build/local/jib_test.go b/pkg/skaffold/build/local/jib_maven_test.go similarity index 78% rename from pkg/skaffold/build/local/jib_test.go rename to pkg/skaffold/build/local/jib_maven_test.go index 94fab894b89..54248ade820 100644 --- a/pkg/skaffold/build/local/jib_test.go +++ b/pkg/skaffold/build/local/jib_maven_test.go @@ -52,22 +52,3 @@ func TestMavenVerifyJibPackageGoal(t *testing.T) { } } } - -func TestGenerateJibImageRef(t *testing.T) { - var testCases = []struct { - workspace string - project string - out string - }{ - {"simple", "", "jibsimple"}, - {"simple", "project", "jibsimple_project"}, - {".", "project", "jib__d8c7cbe8892fe8442b7f6ef42026769ee6a01e67"}, - {"complex/workspace", "project", "jib__965ec099f720d3ccc9c038c21ea4a598c9632883"}, - } - - for _, tt := range testCases { - computed := generateJibImageRef(tt.workspace, tt.project) - - testutil.CheckDeepEqual(t, tt.out, computed) - } -} diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 3183aa28d5e..010e67cc47d 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -20,11 +20,11 @@ import ( "context" "fmt" "io" + "strings" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/pkg/errors" ) @@ -49,64 +49,44 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "generating tag") } - digest, err := b.runBuildForArtifact(ctx, out, artifact) + digestOrImageID, err := b.runBuildForArtifact(ctx, out, artifact, tag) if err != nil { return "", errors.Wrap(err, "build artifact") } - if err := b.retagAndPush(ctx, out, digest, tag, artifact); err != nil { - return "", errors.Wrap(err, "tagging") + if !b.pushImages { + imageID := digestOrImageID + return strings.TrimPrefix(imageID, "sha256:"), nil } - if b.pushImages { + // Jib pushes images directly + if artifact.JibMavenArtifact != nil || artifact.JibGradleArtifact != nil { + digest := digestOrImageID return tag + "@" + digest, nil - } else { - // Here, digest is the imageID - return digest, nil } + + digest, err := b.localDocker.Push(ctx, out, tag) + if err != nil { + return "", errors.Wrap(err, "pushing") + } + return tag + "@" + digest, nil } -func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact) (string, error) { +func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { switch { case artifact.DockerArtifact != nil: - return b.buildDocker(ctx, out, artifact.Workspace, artifact.DockerArtifact) + return b.buildDocker(ctx, out, artifact.Workspace, artifact.DockerArtifact, tag) case artifact.BazelArtifact != nil: - return b.buildBazel(ctx, out, artifact.Workspace, artifact) + return b.buildBazel(ctx, out, artifact.Workspace, artifact.BazelArtifact, tag) case artifact.JibMavenArtifact != nil: - return b.buildJibMaven(ctx, out, artifact.Workspace, artifact) + return b.buildJibMaven(ctx, out, artifact.Workspace, artifact.JibMavenArtifact, tag) case artifact.JibGradleArtifact != nil: - return b.buildJibGradle(ctx, out, artifact.Workspace, artifact) + return b.buildJibGradle(ctx, out, artifact.Workspace, artifact.JibGradleArtifact, tag) default: return "", fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType) } } - -func (b *Builder) retagAndPush(ctx context.Context, out io.Writer, digest string, newTag string, artifact *latest.Artifact) error { - if b.pushImages && (artifact.JibMavenArtifact != nil || artifact.JibGradleArtifact != nil || artifact.BazelArtifact != nil) { - // when pushing images, jib/bazel build them directly to the registry. all we need to do here is add a tag to the remote. - - // NOTE: the digest returned by the builders when in push mode is the digest of the remote image that was built to the registry. - // when adding the tag to the remote, we need to specify the registry it was built to so go-containerregistry knows - // where to look when grabbing the remote image reference. - if err := docker.AddTag(fmt.Sprintf("%s@%s", artifact.ImageName, digest), newTag); err != nil { - return errors.Wrap(err, "tagging image") - } - return nil - } - - if err := b.localDocker.Tag(ctx, digest, newTag); err != nil { - return err - } - - if b.pushImages { - if _, err := b.localDocker.Push(ctx, out, newTag); err != nil { - return errors.Wrap(err, "pushing") - } - } - - return nil -} diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index f1e8d827cb0..2543520843d 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -55,80 +55,64 @@ func TestLocalRun(t *testing.T) { docker.DefaultAuthHelper = testAuthHelper{} var tests = []struct { - description string - api testutil.FakeAPIClient - tagger tag.Tagger - artifacts []*latest.Artifact - expected []build.Artifact - localCluster bool - shouldErr bool + description string + api testutil.FakeAPIClient + tagger tag.Tagger + artifacts []*latest.Artifact + expected []build.Artifact + pushImages bool + shouldErr bool }{ { - description: "single build", + description: "single build (local)", artifacts: []*latest.Artifact{{ ImageName: "gcr.io/test/image", ArtifactType: latest.ArtifactType{ DockerArtifact: &latest.DockerArtifact{}, }}, }, - tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, - expected: []build.Artifact{{ - ImageName: "gcr.io/test/image", - Tag: "gcr.io/test/image:tag", - }}, - }, - { - description: "single build local cluster", - artifacts: []*latest.Artifact{{ - ImageName: "gcr.io/test/image", - ArtifactType: latest.ArtifactType{ - DockerArtifact: &latest.DockerArtifact{}, - }}, - }, - tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, - localCluster: true, + tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, + api: testutil.FakeAPIClient{}, + pushImages: false, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", - Tag: "gcr.io/test/image:tag", + Tag: "1", // ImageID without sha256: prefix }}, }, { - description: "subset build", - tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, + description: "single build (remote)", artifacts: []*latest.Artifact{{ ImageName: "gcr.io/test/image", ArtifactType: latest.ArtifactType{ DockerArtifact: &latest.DockerArtifact{}, }}, }, + tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, + api: testutil.FakeAPIClient{}, + pushImages: true, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", - Tag: "gcr.io/test/image:tag", + Tag: "gcr.io/test/image:tag@sha256:7368613235363a31e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", }}, }, { description: "error image build", + tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, artifacts: []*latest.Artifact{{}}, api: testutil.FakeAPIClient{ ErrImageBuild: true, }, shouldErr: true, }, - { - description: "error image tag", - artifacts: []*latest.Artifact{{}}, - api: testutil.FakeAPIClient{ - ErrImageTag: true, - }, - shouldErr: true, - }, { description: "unkown artifact type", + tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, artifacts: []*latest.Artifact{{}}, shouldErr: true, }, { description: "error image inspect", + tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, artifacts: []*latest.Artifact{{}}, api: testutil.FakeAPIClient{ ErrImageInspect: true, @@ -217,12 +201,13 @@ func TestLocalRun(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { l := Builder{ - cfg: &latest.LocalBuild{}, - localDocker: docker.NewLocalDaemon(&test.api, nil), - localCluster: test.localCluster, + cfg: &latest.LocalBuild{}, + localDocker: docker.NewLocalDaemon(&test.api, nil), + pushImages: test.pushImages, } res, err := l.Build(context.Background(), ioutil.Discard, test.tagger, test.artifacts) + testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, res) }) } From fd242766015523756cb64410a547cc63bef86d6e Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 10:01:53 -0500 Subject: [PATCH 07/13] =?UTF-8?q?Don=E2=80=99t=20pass=20the=20digest=20to?= =?UTF-8?q?=20taggers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Gageot --- .../en/docs/how-tos/templating/_index.md | 8 +--- integration/examples/annotated-skaffold.yaml | 3 -- pkg/skaffold/build/gcb/cloud_build.go | 4 +- pkg/skaffold/build/kaniko/kaniko.go | 4 +- pkg/skaffold/build/local/local.go | 4 +- pkg/skaffold/build/local/local_test.go | 2 +- pkg/skaffold/build/tag/custom.go | 4 +- pkg/skaffold/build/tag/custom_test.go | 4 +- pkg/skaffold/build/tag/date_time.go | 4 +- pkg/skaffold/build/tag/date_time_test.go | 4 +- pkg/skaffold/build/tag/env_template.go | 36 +++++++---------- pkg/skaffold/build/tag/env_template_test.go | 39 ++++++++++-------- pkg/skaffold/build/tag/git_commit.go | 35 ++++------------ pkg/skaffold/build/tag/git_commit_test.go | 21 +++++----- pkg/skaffold/build/tag/sha256.go | 14 +------ pkg/skaffold/build/tag/sha256_test.go | 40 ++----------------- pkg/skaffold/build/tag/tag.go | 7 +--- pkg/skaffold/deploy/helm.go | 19 ++++++++- 18 files changed, 90 insertions(+), 162 deletions(-) diff --git a/docs/content/en/docs/how-tos/templating/_index.md b/docs/content/en/docs/how-tos/templating/_index.md index 0c6de064352..364ff9286c0 100755 --- a/docs/content/en/docs/how-tos/templating/_index.md +++ b/docs/content/en/docs/how-tos/templating/_index.md @@ -20,9 +20,5 @@ List of fields that support templating: List of variables that are available for templating: -* all environment variables passed to the Skaffold process as startup -* `IMAGE_NAME` - the artifacts' image name - the [image name rewriting](/docs/concepts/#image-repository-handling) acts after the template was calculated -* `DIGEST` - the image digest calculated by the docker registry after pushing the image -* if `DIGEST` is of format `algo:hex`, `DIGEST_ALGO` and `DIGEST_HEX` parts correspond to the parts of the string otherwise `DIGEST_HEX`=`DIGEST` is set - - +* all environment variables passed to the skaffold process as startup +* `IMAGE_NAME` - the artifacts' image name - the [image name rewriting](/docs/concepts/#image-repository-handling) acts after the template was calculated diff --git a/integration/examples/annotated-skaffold.yaml b/integration/examples/annotated-skaffold.yaml index e3e890efb09..42730680d53 100644 --- a/integration/examples/annotated-skaffold.yaml +++ b/integration/examples/annotated-skaffold.yaml @@ -19,9 +19,6 @@ build: # The template is compiled and executed against the current environment, # with those variables injected: # IMAGE_NAME | Name of the image being built, as supplied in the artifacts section. - # DIGEST | Digest of the newly built image. For eg. `sha256:27ffc7f352665cc50ae3cbcc4b2725e36062f1b38c611b6f95d6df9a7510de23`. - # DIGEST_ALGO | Algorithm used by the digest: For eg. `sha256`. - # DIGEST_HEX | Digest of the newly built image. For eg. `27ffc7f352665cc50ae3cbcc4b2725e36062f1b38c611b6f95d6df9a7510de23`. # Example # envTemplate: # template: "{{.RELEASE}}-{{.IMAGE_NAME}}" diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 7ffa9b6406c..57e81d94112 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -48,9 +48,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ - ImageName: artifact.ImageName, - }) + tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, artifact.ImageName) if err != nil { return "", errors.Wrap(err, "generating tag") } diff --git a/pkg/skaffold/build/kaniko/kaniko.go b/pkg/skaffold/build/kaniko/kaniko.go index db294aa7e65..17d7a517460 100644 --- a/pkg/skaffold/build/kaniko/kaniko.go +++ b/pkg/skaffold/build/kaniko/kaniko.go @@ -46,9 +46,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifactWithKaniko(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ - ImageName: artifact.ImageName, - }) + tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, artifact.ImageName) if err != nil { return "", errors.Wrap(err, "generating tag") } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 010e67cc47d..339a68076a8 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -42,9 +42,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, a } func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.Tagger, artifact *latest.Artifact) (string, error) { - tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, tag.Options{ - ImageName: artifact.ImageName, - }) + tag, err := tagger.GenerateFullyQualifiedImageName(artifact.Workspace, artifact.ImageName) if err != nil { return "", errors.Wrap(err, "generating tag") } diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index 2543520843d..f300da01f64 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -35,7 +35,7 @@ type FakeTagger struct { Err error } -func (f *FakeTagger) GenerateFullyQualifiedImageName(workingDir string, tagOpts tag.Options) (string, error) { +func (f *FakeTagger) GenerateFullyQualifiedImageName(string, string) (string, error) { return f.Out, f.Err } diff --git a/pkg/skaffold/build/tag/custom.go b/pkg/skaffold/build/tag/custom.go index 44b2aa41957..ffc42aa0f37 100644 --- a/pkg/skaffold/build/tag/custom.go +++ b/pkg/skaffold/build/tag/custom.go @@ -34,11 +34,11 @@ func (c *CustomTag) Labels() map[string]string { } // GenerateFullyQualifiedImageName tags an image with the custom tag -func (c *CustomTag) GenerateFullyQualifiedImageName(workingDir string, opts Options) (string, error) { +func (c *CustomTag) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { tag := c.Tag if tag == "" { return "", errors.New("custom tag not provided") } - return fmt.Sprintf("%s:%s", opts.ImageName, tag), nil + return fmt.Sprintf("%s:%s", imageName, tag), nil } diff --git a/pkg/skaffold/build/tag/custom_test.go b/pkg/skaffold/build/tag/custom_test.go index 34c793f152d..e5cdb4262c1 100644 --- a/pkg/skaffold/build/tag/custom_test.go +++ b/pkg/skaffold/build/tag/custom_test.go @@ -27,9 +27,7 @@ func TestCustomTag_GenerateFullyQualifiedImageName(t *testing.T) { Tag: "1.2.3-beta", } - tag, err := c.GenerateFullyQualifiedImageName(".", Options{ - ImageName: "test", - }) + tag, err := c.GenerateFullyQualifiedImageName(".", "test") testutil.CheckErrorAndDeepEqual(t, false, err, "test:1.2.3-beta", tag) } diff --git a/pkg/skaffold/build/tag/date_time.go b/pkg/skaffold/build/tag/date_time.go index b60f1d2f63a..d94bd116239 100644 --- a/pkg/skaffold/build/tag/date_time.go +++ b/pkg/skaffold/build/tag/date_time.go @@ -49,7 +49,7 @@ func (tagger *dateTimeTagger) Labels() map[string]string { } // GenerateFullyQualifiedImageName tags an image with the supplied image name and the current timestamp -func (tagger *dateTimeTagger) GenerateFullyQualifiedImageName(workingDir string, opts Options) (string, error) { +func (tagger *dateTimeTagger) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { format := tagTime if len(tagger.Format) > 0 { format = tagger.Format @@ -65,5 +65,5 @@ func (tagger *dateTimeTagger) GenerateFullyQualifiedImageName(workingDir string, return "", fmt.Errorf("bad timezone provided: \"%s\", error: %s", timezone, err) } - return fmt.Sprintf("%s:%s", opts.ImageName, tagger.timeFn().In(loc).Format(format)), nil + return fmt.Sprintf("%s:%s", imageName, tagger.timeFn().In(loc).Format(format)), nil } diff --git a/pkg/skaffold/build/tag/date_time_test.go b/pkg/skaffold/build/tag/date_time_test.go index f2d2d59f506..3314d1d2445 100644 --- a/pkg/skaffold/build/tag/date_time_test.go +++ b/pkg/skaffold/build/tag/date_time_test.go @@ -66,9 +66,7 @@ func TestDateTime_GenerateFullyQualifiedImageName(t *testing.T) { TimeZone: test.timezone, timeFn: func() time.Time { return test.buildTime }, } - tag, err := c.GenerateFullyQualifiedImageName(".", Options{ - ImageName: test.imageName, - }) + tag, err := c.GenerateFullyQualifiedImageName(".", test.imageName) testutil.CheckErrorAndDeepEqual(t, false, err, test.want, tag) }) diff --git a/pkg/skaffold/build/tag/env_template.go b/pkg/skaffold/build/tag/env_template.go index 306d7a160b7..6b9053e03c9 100644 --- a/pkg/skaffold/build/tag/env_template.go +++ b/pkg/skaffold/build/tag/env_template.go @@ -32,10 +32,21 @@ type envTemplateTagger struct { // NewEnvTemplateTagger creates a new envTemplateTagger func NewEnvTemplateTagger(t string) (Tagger, error) { + if strings.Contains(t, "{{.DIGEST}}") { + return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be apppended to image tags") + } + if strings.Contains(t, "{{.DIGEST_ALGO}}") { + return nil, errors.New("{{.DIGEST_ALGO}} is deprecated, image digest will automatically be apppended to image tags") + } + if strings.Contains(t, "{{.DIGEST_HEX}}") { + return nil, errors.New("{{.DIGEST_HEX}} is deprecated, image digest will automatically be apppended to image tags") + } + tmpl, err := util.ParseEnvTemplate(t) if err != nil { return nil, errors.Wrap(err, "parsing template") } + return &envTemplateTagger{ Template: tmpl, }, nil @@ -48,25 +59,8 @@ func (t *envTemplateTagger) Labels() map[string]string { } // GenerateFullyQualifiedImageName tags an image with the custom tag -func (t *envTemplateTagger) GenerateFullyQualifiedImageName(workingDir string, opts Options) (string, error) { - customMap := CreateEnvVarMap(opts.ImageName, opts.Digest) - return util.ExecuteEnvTemplate(t.Template, customMap) -} - -// CreateEnvVarMap creates a set of environment variables for use in Templates from the given -// image name and digest -func CreateEnvVarMap(imageName string, digest string) map[string]string { - customMap := map[string]string{} - customMap["IMAGE_NAME"] = imageName - customMap["DIGEST"] = digest - if digest != "" { - names := strings.SplitN(digest, ":", 2) - if len(names) >= 2 { - customMap["DIGEST_ALGO"] = names[0] - customMap["DIGEST_HEX"] = names[1] - } else { - customMap["DIGEST_HEX"] = digest - } - } - return customMap +func (t *envTemplateTagger) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { + return util.ExecuteEnvTemplate(t.Template, map[string]string{ + "IMAGE_NAME": imageName, + }) } diff --git a/pkg/skaffold/build/tag/env_template_test.go b/pkg/skaffold/build/tag/env_template_test.go index ef45c6e16c8..a48e7c1b66e 100644 --- a/pkg/skaffold/build/tag/env_template_test.go +++ b/pkg/skaffold/build/tag/env_template_test.go @@ -28,23 +28,21 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { name string template string imageName string - digest string env []string expected string + shoudErr bool }{ { name: "empty env", - template: "{{.IMAGE_NAME}}:{{.DIGEST}}", + template: "{{.IMAGE_NAME}}", imageName: "foo", - digest: "bar", - expected: "foo:bar", + expected: "foo", }, { name: "env", template: "{{.FOO}}-{{.BAZ}}:latest", env: []string{"FOO=BAR", "BAZ=BAT"}, imageName: "foo", - digest: "bar", expected: "BAR-BAT:latest", }, { @@ -52,15 +50,25 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { template: "{{.IMAGE_NAME}}-{{.FROM_ENV}}:latest", env: []string{"FROM_ENV=FOO", "IMAGE_NAME=BAT"}, imageName: "image_name", - digest: "bar", expected: "image_name-FOO:latest", }, { - name: "digest algo hex", - template: "{{.IMAGE_NAME}}:{{.DIGEST_ALGO}}-{{.DIGEST_HEX}}", + name: "digest is not supported anymore", + template: "{{.IMAGE_NAME}}@{{.DIGEST}}", imageName: "foo", - digest: "sha256:abcd", - expected: "foo:sha256-abcd", + shoudErr: true, + }, + { + name: "digest algo is not supported anymore", + template: "{{.IMAGE_NAME}}@{{.DIGEST_ALGO}}", + imageName: "foo", + shoudErr: true, + }, + { + name: "digest hex is not supported anymore", + template: "{{.IMAGE_NAME}}@{{.DIGEST_HEX}}", + imageName: "foo", + shoudErr: true, }, } for _, test := range tests { @@ -70,14 +78,13 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { } c, err := NewEnvTemplateTagger(test.template) - testutil.CheckError(t, false, err) + testutil.CheckError(t, test.shoudErr, err) - got, err := c.GenerateFullyQualifiedImageName("", Options{ - ImageName: test.imageName, - Digest: test.digest, - }) + if !test.shoudErr { + got, err := c.GenerateFullyQualifiedImageName("", test.imageName) - testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, got) + testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, got) + } }) } } diff --git a/pkg/skaffold/build/tag/git_commit.go b/pkg/skaffold/build/tag/git_commit.go index c7d1675cff2..7510ae5a052 100644 --- a/pkg/skaffold/build/tag/git_commit.go +++ b/pkg/skaffold/build/tag/git_commit.go @@ -20,7 +20,6 @@ import ( "bytes" "fmt" "os/exec" - "strings" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" @@ -39,10 +38,11 @@ func (c *GitCommit) Labels() map[string]string { } // GenerateFullyQualifiedImageName tags an image with the supplied image name and the git commit. -func (c *GitCommit) GenerateFullyQualifiedImageName(workingDir string, opts Options) (string, error) { +func (c *GitCommit) GenerateFullyQualifiedImageName(workingDir string, imageName string) (string, error) { hash, err := runGit(workingDir, "rev-parse", "--short", "HEAD") if err != nil { - return fallbackOnDigest(opts, err), nil + logrus.Warnln("Unable to find git commit:", err) + return fmt.Sprintf("%s:dirty", imageName), nil } changes, err := runGit(workingDir, "status", ".", "--porcelain") @@ -51,13 +51,16 @@ func (c *GitCommit) GenerateFullyQualifiedImageName(workingDir string, opts Opti } if len(changes) > 0 { - return dirtyTag(hash, opts), nil + return fmt.Sprintf("%s:%s-dirty", imageName, hash), nil } // Ignore error. It means there's no tag. tag, _ := runGit(workingDir, "describe", "--tags", "--exact-match") + if len(tag) > 0 { + return fmt.Sprintf("%s:%s", imageName, tag), nil + } - return commitOrTag(hash, tag, opts), nil + return fmt.Sprintf("%s:%s", imageName, hash), nil } func runGit(workingDir string, arg ...string) (string, error) { @@ -71,25 +74,3 @@ func runGit(workingDir string, arg ...string) (string, error) { return string(bytes.TrimSpace(out)), nil } - -func commitOrTag(currentTag string, tag string, opts Options) string { - if len(tag) > 0 { - currentTag = tag - } - - return fmt.Sprintf("%s:%s", opts.ImageName, currentTag) -} - -func shortDigest(opts Options) string { - return strings.TrimPrefix(opts.Digest, "sha256:")[0:7] -} - -func dirtyTag(currentTag string, opts Options) string { - return fmt.Sprintf("%s:%s-dirty-%s", opts.ImageName, currentTag, shortDigest(opts)) -} - -func fallbackOnDigest(opts Options, err error) string { - logrus.Warnln("Using digest instead of git commit:", err) - - return fmt.Sprintf("%s:dirty-%s", opts.ImageName, shortDigest(opts)) -} diff --git a/pkg/skaffold/build/tag/git_commit_test.go b/pkg/skaffold/build/tag/git_commit_test.go index 1a4cb56723c..2bf8495ff9a 100644 --- a/pkg/skaffold/build/tag/git_commit_test.go +++ b/pkg/skaffold/build/tag/git_commit_test.go @@ -68,7 +68,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "dirty", - expectedName: "test:eefe1b9-dirty-abababa", + expectedName: "test:eefe1b9-dirty", createGitRepo: func(dir string) { gitInit(t, dir). write("source.go", []byte("code")). @@ -79,7 +79,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "ignore tag when dirty", - expectedName: "test:eefe1b9-dirty-abababa", + expectedName: "test:eefe1b9-dirty", createGitRepo: func(dir string) { gitInit(t, dir). write("source.go", []byte("code")). @@ -91,7 +91,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "untracked", - expectedName: "test:eefe1b9-dirty-abababa", + expectedName: "test:eefe1b9-dirty", createGitRepo: func(dir string) { gitInit(t, dir). write("source.go", []byte("code")). @@ -116,7 +116,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "deleted file", - expectedName: "test:279d53f-dirty-abababa", + expectedName: "test:279d53f-dirty", createGitRepo: func(dir string) { gitInit(t, dir). write("source1.go", []byte("code1")). @@ -128,7 +128,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "rename", - expectedName: "test:eefe1b9-dirty-abababa", + expectedName: "test:eefe1b9-dirty", createGitRepo: func(dir string) { gitInit(t, dir). write("source.go", []byte("code")). @@ -186,7 +186,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "updated artifact in dirty repo", - expectedName: "test:0c60cb8-dirty-abababa", + expectedName: "test:0c60cb8-dirty", createGitRepo: func(dir string) { gitInit(t, dir). mkdir("artifact1").write("artifact1/source.go", []byte("code")). @@ -199,14 +199,14 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { }, { description: "non git repo", - expectedName: "test:dirty-abababa", + expectedName: "test:dirty", createGitRepo: func(dir string) { ioutil.WriteFile(filepath.Join(dir, "source.go"), []byte("code"), os.ModePerm) }, }, { description: "git repo with no commit", - expectedName: "test:dirty-abababa", + expectedName: "test:dirty", createGitRepo: func(dir string) { gitInit(t, dir) }, @@ -223,10 +223,7 @@ func TestGitCommit_GenerateFullyQualifiedImageName(t *testing.T) { c := &GitCommit{} - name, err := c.GenerateFullyQualifiedImageName(workspace, Options{ - ImageName: "test", - Digest: "sha256:ababababababababababa", - }) + name, err := c.GenerateFullyQualifiedImageName(workspace, "test") testutil.CheckErrorAndDeepEqual(t, tt.shouldErr, err, tt.expectedName, name) }) diff --git a/pkg/skaffold/build/tag/sha256.go b/pkg/skaffold/build/tag/sha256.go index 153db9a6478..f5d47fe8907 100644 --- a/pkg/skaffold/build/tag/sha256.go +++ b/pkg/skaffold/build/tag/sha256.go @@ -17,9 +17,6 @@ limitations under the License. package tag import ( - "fmt" - "strings" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" ) @@ -33,13 +30,6 @@ func (c *ChecksumTagger) Labels() map[string]string { } } -// GenerateFullyQualifiedImageName tags an image with the supplied image name and the sha256 checksum of the image -func (c *ChecksumTagger) GenerateFullyQualifiedImageName(workingDir string, opts Options) (string, error) { - digest := opts.Digest - sha256 := strings.TrimPrefix(opts.Digest, "sha256:") - if sha256 == digest { - return "", fmt.Errorf("digest wrong format: %s, expected sha256:", digest) - } - - return fmt.Sprintf("%s:%s", opts.ImageName, sha256), nil +func (c *ChecksumTagger) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { + return imageName, nil } diff --git a/pkg/skaffold/build/tag/sha256_test.go b/pkg/skaffold/build/tag/sha256_test.go index b82bd144cce..877a705fe46 100644 --- a/pkg/skaffold/build/tag/sha256_test.go +++ b/pkg/skaffold/build/tag/sha256_test.go @@ -23,43 +23,9 @@ import ( ) func TestGenerateFullyQualifiedImageName(t *testing.T) { - var tests = []struct { - description string - imageName string - digest string - expected string - shouldErr bool - }{ - { - description: "no error", - imageName: "test", - digest: "sha256:12345abcde", - expected: "test:12345abcde", - }, - { - description: "wrong digest format", - imageName: "test", - digest: "wrong:digest:format", - shouldErr: true, - }, - { - description: "wrong digest format no colon", - imageName: "test", - digest: "sha256", - shouldErr: true, - }, - } + c := &ChecksumTagger{} - for _, test := range tests { - t.Run(test.description, func(t *testing.T) { - c := &ChecksumTagger{} + tag, err := c.GenerateFullyQualifiedImageName(".", "img:tag") - tag, err := c.GenerateFullyQualifiedImageName(".", Options{ - ImageName: test.imageName, - Digest: test.digest, - }) - - testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, tag) - }) - } + testutil.CheckErrorAndDeepEqual(t, false, err, "img:tag", tag) } diff --git a/pkg/skaffold/build/tag/tag.go b/pkg/skaffold/build/tag/tag.go index 83420f751f5..f1cf91aa411 100644 --- a/pkg/skaffold/build/tag/tag.go +++ b/pkg/skaffold/build/tag/tag.go @@ -20,10 +20,5 @@ package tag type Tagger interface { Labels() map[string]string - GenerateFullyQualifiedImageName(workingDir string, tagOpts Options) (string, error) -} - -type Options struct { - ImageName string - Digest string + GenerateFullyQualifiedImageName(workingDir string, imageName string) (string, error) } diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go index 1ad1c5b18db..b279836e507 100644 --- a/pkg/skaffold/deploy/helm.go +++ b/pkg/skaffold/deploy/helm.go @@ -30,7 +30,6 @@ import ( "strings" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" @@ -239,7 +238,7 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates if idx > 0 { suffix = strconv.Itoa(idx + 1) } - m := tag.CreateEnvVarMap(b.ImageName, extractTag(b.Tag)) + m := createEnvVarMap(b.ImageName, extractTag(b.Tag)) for k, v := range m { envMap[k+suffix] = v } @@ -270,6 +269,22 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates return h.getDeployResults(ctx, ns, releaseName), helmErr } +func createEnvVarMap(imageName string, digest string) map[string]string { + customMap := map[string]string{} + customMap["IMAGE_NAME"] = imageName + customMap["DIGEST"] = digest + if digest != "" { + names := strings.SplitN(digest, ":", 2) + if len(names) >= 2 { + customMap["DIGEST_ALGO"] = names[0] + customMap["DIGEST_HEX"] = names[1] + } else { + customMap["DIGEST_HEX"] = digest + } + } + return customMap +} + // imageName if the given string includes a fully qualified docker image name then lets trim just the tag part out func extractTag(imageName string) string { idx := strings.LastIndex(imageName, "/") From 923f4de4455c4e4d3aeaaf752bf1eca4a5851fa1 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 Jan 2019 13:25:08 -0500 Subject: [PATCH 08/13] Fix the case of the local dev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Where digest is not available and imageID can’t be used in k8s manifests. Signed-off-by: David Gageot --- pkg/skaffold/build/local/local.go | 12 +++++++++++- pkg/skaffold/build/local/local_test.go | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 339a68076a8..0aa07c51cb7 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -54,7 +54,17 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T if !b.pushImages { imageID := digestOrImageID - return strings.TrimPrefix(imageID, "sha256:"), nil + + // k8s doesn't recognize the imageID or any combination of the image name + // suffixed with the imageID, as a valid image name. + // So, the solution we chose is to create a tag, just for Skaffold, from + // the imageID, and use that in the manifests. + uniqueTag := artifact.ImageName + ":" + strings.TrimPrefix(imageID, "sha256:") + if err := b.localDocker.Tag(ctx, imageID, uniqueTag); err != nil { + return "", err + } + + return uniqueTag, nil } // Jib pushes images directly diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index f300da01f64..483e5d7dd24 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -76,7 +76,7 @@ func TestLocalRun(t *testing.T) { pushImages: false, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", - Tag: "1", // ImageID without sha256: prefix + Tag: "gcr.io/test/image:1", }}, }, { From 31bde8703aceac48d6cb1392f2fdee1e62c01307 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Sat, 19 Jan 2019 15:37:23 -0500 Subject: [PATCH 09/13] =?UTF-8?q?Don=E2=80=99t=20use=20local=20Docker=20to?= =?UTF-8?q?=20push=20Bazel=20images?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #1349 Signed-off-by: David Gageot --- pkg/skaffold/build/local/bazel.go | 6 +++--- pkg/skaffold/build/local/local.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/skaffold/build/local/bazel.go b/pkg/skaffold/build/local/bazel.go index 1f70f2a7697..1605515cf79 100644 --- a/pkg/skaffold/build/local/bazel.go +++ b/pkg/skaffold/build/local/bazel.go @@ -61,8 +61,7 @@ func (b *Builder) buildBazel(ctx context.Context, out io.Writer, workspace strin return pushImage(tarPath, tag) } - bazelTag := buildImageTag(a.BuildTarget) - return b.loadImage(ctx, out, tarPath, bazelTag, tag) + return b.loadImage(ctx, out, tarPath, a, tag) } func pushImage(tarPath, tag string) (string, error) { @@ -88,13 +87,14 @@ func pushImage(tarPath, tag string) (string, error) { return docker.RemoteDigest(tag) } -func (b *Builder) loadImage(ctx context.Context, out io.Writer, tarPath string, bazelTag string, tag string) (string, error) { +func (b *Builder) loadImage(ctx context.Context, out io.Writer, tarPath string, a *latest.BazelArtifact, tag string) (string, error) { imageTar, err := os.Open(tarPath) if err != nil { return "", errors.Wrap(err, "opening image tarball") } defer imageTar.Close() + bazelTag := buildImageTag(a.BuildTarget) imageID, err := b.localDocker.Load(ctx, out, imageTar, bazelTag) if err != nil { return "", errors.Wrap(err, "loading image into docker daemon") diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 0aa07c51cb7..d2cf25ae85d 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -68,7 +68,7 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T } // Jib pushes images directly - if artifact.JibMavenArtifact != nil || artifact.JibGradleArtifact != nil { + if artifact.JibMavenArtifact != nil || artifact.JibGradleArtifact != nil || artifact.BazelArtifact != nil { digest := digestOrImageID return tag + "@" + digest, nil } From 66eb95fadcc6147bbe2679be5855f22390651ef3 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Sat, 19 Jan 2019 17:51:06 -0500 Subject: [PATCH 10/13] Docker build should push images itself Signed-off-by: David Gageot --- pkg/skaffold/build/local/docker.go | 49 +++++++++++++++++++----------- pkg/skaffold/build/local/local.go | 32 +++++++------------ 2 files changed, 43 insertions(+), 38 deletions(-) diff --git a/pkg/skaffold/build/local/docker.go b/pkg/skaffold/build/local/docker.go index 7a8b3a1704d..2d95516b166 100644 --- a/pkg/skaffold/build/local/docker.go +++ b/pkg/skaffold/build/local/docker.go @@ -33,30 +33,45 @@ func (b *Builder) buildDocker(ctx context.Context, out io.Writer, workspace stri return "", errors.Wrap(err, "pulling cache-from images") } + var ( + imageID string + err error + ) + if b.cfg.UseDockerCLI || b.cfg.UseBuildkit { - dockerfilePath, err := docker.NormalizeDockerfilePath(workspace, a.DockerfilePath) - if err != nil { - return "", errors.Wrap(err, "normalizing dockerfile path") - } + imageID, err = b.dockerCLIBuild(ctx, out, workspace, a, tag) + } else { + imageID, err = b.localDocker.Build(ctx, out, workspace, a, tag) + } - args := []string{"build", workspace, "--file", dockerfilePath, "-t", tag} - args = append(args, docker.GetBuildArgs(a)...) + if b.pushImages { + return b.localDocker.Push(ctx, out, tag) + } - cmd := exec.CommandContext(ctx, "docker", args...) - if b.cfg.UseBuildkit { - cmd.Env = append(os.Environ(), "DOCKER_BUILDKIT=1") - } - cmd.Stdout = out - cmd.Stderr = out + return imageID, err +} - if err := util.RunCmd(cmd); err != nil { - return "", errors.Wrap(err, "running build") - } +func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, tag string) (string, error) { + dockerfilePath, err := docker.NormalizeDockerfilePath(workspace, a.DockerfilePath) + if err != nil { + return "", errors.Wrap(err, "normalizing dockerfile path") + } + + args := []string{"build", workspace, "--file", dockerfilePath, "-t", tag} + args = append(args, docker.GetBuildArgs(a)...) + + cmd := exec.CommandContext(ctx, "docker", args...) + if b.cfg.UseBuildkit { + cmd.Env = append(os.Environ(), "DOCKER_BUILDKIT=1") + } + cmd.Stdout = out + cmd.Stderr = out - return b.localDocker.ImageID(ctx, tag) + if err := util.RunCmd(cmd); err != nil { + return "", errors.Wrap(err, "running build") } - return b.localDocker.Build(ctx, out, workspace, a, tag) + return b.localDocker.ImageID(ctx, tag) } func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *latest.DockerArtifact) error { diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index d2cf25ae85d..5b8bf802bf0 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -52,32 +52,22 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T return "", errors.Wrap(err, "build artifact") } - if !b.pushImages { - imageID := digestOrImageID - - // k8s doesn't recognize the imageID or any combination of the image name - // suffixed with the imageID, as a valid image name. - // So, the solution we chose is to create a tag, just for Skaffold, from - // the imageID, and use that in the manifests. - uniqueTag := artifact.ImageName + ":" + strings.TrimPrefix(imageID, "sha256:") - if err := b.localDocker.Tag(ctx, imageID, uniqueTag); err != nil { - return "", err - } - - return uniqueTag, nil - } - - // Jib pushes images directly - if artifact.JibMavenArtifact != nil || artifact.JibGradleArtifact != nil || artifact.BazelArtifact != nil { + if b.pushImages { digest := digestOrImageID return tag + "@" + digest, nil } - digest, err := b.localDocker.Push(ctx, out, tag) - if err != nil { - return "", errors.Wrap(err, "pushing") + // k8s doesn't recognize the imageID or any combination of the image name + // suffixed with the imageID, as a valid image name. + // So, the solution we chose is to create a tag, just for Skaffold, from + // the imageID, and use that in the manifests. + imageID := digestOrImageID + uniqueTag := artifact.ImageName + ":" + strings.TrimPrefix(imageID, "sha256:") + if err := b.localDocker.Tag(ctx, imageID, uniqueTag); err != nil { + return "", err } - return tag + "@" + digest, nil + + return uniqueTag, nil } func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { From 1b02585f4d3d53ac68928e86d31d4079461acbfc Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 23 Jan 2019 09:56:21 +0100 Subject: [PATCH 11/13] Fix test Signed-off-by: David Gageot --- pkg/skaffold/build/local/local_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index 483e5d7dd24..b150fc43954 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -144,7 +144,7 @@ func TestLocalRun(t *testing.T) { tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", - Tag: "gcr.io/test/image:tag", + Tag: "gcr.io/test/image:1", }}, }, { @@ -161,7 +161,7 @@ func TestLocalRun(t *testing.T) { tagger: &FakeTagger{Out: "gcr.io/test/image:tag"}, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", - Tag: "gcr.io/test/image:tag", + Tag: "gcr.io/test/image:1", }}, }, { From 1f9f4cb8f7a08db5ae89605fdba2b2a5873a6c7f Mon Sep 17 00:00:00 2001 From: David Gageot Date: Mon, 4 Feb 2019 11:20:56 +0100 Subject: [PATCH 12/13] Ignore proper usage of {{.DIGEST}} and print warning Signed-off-by: David Gageot --- pkg/skaffold/build/tag/env_template.go | 51 ++++++++++---- pkg/skaffold/build/tag/env_template_test.go | 77 +++++++++++++++------ 2 files changed, 93 insertions(+), 35 deletions(-) diff --git a/pkg/skaffold/build/tag/env_template.go b/pkg/skaffold/build/tag/env_template.go index 6b9053e03c9..e6b0656eb6f 100644 --- a/pkg/skaffold/build/tag/env_template.go +++ b/pkg/skaffold/build/tag/env_template.go @@ -23,8 +23,23 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) +// for testing +var warner Warner = &logrusWarner{} + +// Warner shows warnings. +type Warner interface { + Warnf(format string, args ...interface{}) +} + +type logrusWarner struct{} + +func (l *logrusWarner) Warnf(format string, args ...interface{}) { + logrus.Warnf(format, args...) +} + // envTemplateTagger implements Tagger type envTemplateTagger struct { Template *template.Template @@ -32,16 +47,6 @@ type envTemplateTagger struct { // NewEnvTemplateTagger creates a new envTemplateTagger func NewEnvTemplateTagger(t string) (Tagger, error) { - if strings.Contains(t, "{{.DIGEST}}") { - return nil, errors.New("{{.DIGEST}} is deprecated, image digest will automatically be apppended to image tags") - } - if strings.Contains(t, "{{.DIGEST_ALGO}}") { - return nil, errors.New("{{.DIGEST_ALGO}} is deprecated, image digest will automatically be apppended to image tags") - } - if strings.Contains(t, "{{.DIGEST_HEX}}") { - return nil, errors.New("{{.DIGEST_HEX}} is deprecated, image digest will automatically be apppended to image tags") - } - tmpl, err := util.ParseEnvTemplate(t) if err != nil { return nil, errors.Wrap(err, "parsing template") @@ -60,7 +65,29 @@ func (t *envTemplateTagger) Labels() map[string]string { // GenerateFullyQualifiedImageName tags an image with the custom tag func (t *envTemplateTagger) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { - return util.ExecuteEnvTemplate(t.Template, map[string]string{ - "IMAGE_NAME": imageName, + tag, err := util.ExecuteEnvTemplate(t.Template, map[string]string{ + "IMAGE_NAME": imageName, + "DIGEST": "[DEPRECATED_DIGEST]", + "DIGEST_ALGO": "[DEPRECATED_DIGEST_ALGO]", + "DIGEST_HEX": "[DEPRECATED_DIGEST_HEX]", }) + if err != nil { + return "", err + } + + if strings.Contains(tag, "[DEPRECATED_DIGEST]") || + strings.Contains(tag, "[DEPRECATED_DIGEST_ALGO]") || + strings.Contains(tag, "[DEPRECATED_DIGEST_HEX]") { + warner.Warnf("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags") + + switch { + case strings.HasSuffix(tag, "@[DEPRECATED_DIGEST]"): + tag = strings.TrimSuffix(tag, "@[DEPRECATED_DIGEST]") + + case strings.HasSuffix(tag, "@[DEPRECATED_DIGEST_ALGO]:[DEPRECATED_DIGEST_HEX]"): + tag = strings.TrimSuffix(tag, "@[DEPRECATED_DIGEST_ALGO]:[DEPRECATED_DIGEST_HEX]") + } + } + + return tag, nil } diff --git a/pkg/skaffold/build/tag/env_template_test.go b/pkg/skaffold/build/tag/env_template_test.go index a48e7c1b66e..fbeac1b1c01 100644 --- a/pkg/skaffold/build/tag/env_template_test.go +++ b/pkg/skaffold/build/tag/env_template_test.go @@ -17,20 +17,31 @@ limitations under the License. package tag import ( + "fmt" + "sort" "testing" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" ) +type fakeWarner struct { + warnings []string +} + +func (l *fakeWarner) Warnf(format string, args ...interface{}) { + l.warnings = append(l.warnings, fmt.Sprintf(format, args...)) + sort.Strings(l.warnings) +} + func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { tests := []struct { - name string - template string - imageName string - env []string - expected string - shoudErr bool + name string + template string + imageName string + env []string + expected string + expectedWarnings []string }{ { name: "empty env", @@ -53,22 +64,39 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { expected: "image_name-FOO:latest", }, { - name: "digest is not supported anymore", - template: "{{.IMAGE_NAME}}@{{.DIGEST}}", - imageName: "foo", - shoudErr: true, + name: "ignore @{{.DIGEST}} suffix", + template: "{{.IMAGE_NAME}}:tag@{{.DIGEST}}", + imageName: "foo", + expected: "foo:tag", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, }, { - name: "digest algo is not supported anymore", - template: "{{.IMAGE_NAME}}@{{.DIGEST_ALGO}}", - imageName: "foo", - shoudErr: true, + name: "ignore @{{.DIGEST_ALGO}}:{{.DIGEST_HEX}} suffix", + template: "{{.IMAGE_NAME}}:tag@{{.DIGEST_ALGO}}:{{.DIGEST_HEX}}", + imageName: "image_name", + expected: "image_name:tag", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, }, { - name: "digest hex is not supported anymore", - template: "{{.IMAGE_NAME}}@{{.DIGEST_HEX}}", - imageName: "foo", - shoudErr: true, + name: "digest is deprecated", + template: "{{.IMAGE_NAME}}:{{.DIGEST}}", + imageName: "foo", + expected: "foo:[DEPRECATED_DIGEST]", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + }, + { + name: "digest algo is deprecated", + template: "{{.IMAGE_NAME}}:{{.DIGEST_ALGO}}", + imageName: "foo", + expected: "foo:[DEPRECATED_DIGEST_ALGO]", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + }, + { + name: "digest hex is deprecated", + template: "{{.IMAGE_NAME}}:{{.DIGEST_HEX}}", + imageName: "foo", + expected: "foo:[DEPRECATED_DIGEST_HEX]", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, }, } for _, test := range tests { @@ -77,14 +105,17 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { return test.env } + defer func(w Warner) { warner = w }(warner) + fakeWarner := &fakeWarner{} + warner = fakeWarner + c, err := NewEnvTemplateTagger(test.template) - testutil.CheckError(t, test.shoudErr, err) + testutil.CheckError(t, false, err) - if !test.shoudErr { - got, err := c.GenerateFullyQualifiedImageName("", test.imageName) + got, err := c.GenerateFullyQualifiedImageName("", test.imageName) - testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, got) - } + testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, got) + testutil.CheckDeepEqual(t, test.expectedWarnings, fakeWarner.warnings) }) } } From 5cee6893c6d217ad3441e0096102062b9dc8c3e3 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Tue, 5 Feb 2019 16:44:53 +0100 Subject: [PATCH 13/13] Produce valid image names Signed-off-by: David Gageot --- pkg/skaffold/build/tag/env_template.go | 22 ++++++++++----------- pkg/skaffold/build/tag/env_template_test.go | 16 +++++++-------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/skaffold/build/tag/env_template.go b/pkg/skaffold/build/tag/env_template.go index e6b0656eb6f..8a852e86693 100644 --- a/pkg/skaffold/build/tag/env_template.go +++ b/pkg/skaffold/build/tag/env_template.go @@ -67,25 +67,25 @@ func (t *envTemplateTagger) Labels() map[string]string { func (t *envTemplateTagger) GenerateFullyQualifiedImageName(workingDir, imageName string) (string, error) { tag, err := util.ExecuteEnvTemplate(t.Template, map[string]string{ "IMAGE_NAME": imageName, - "DIGEST": "[DEPRECATED_DIGEST]", - "DIGEST_ALGO": "[DEPRECATED_DIGEST_ALGO]", - "DIGEST_HEX": "[DEPRECATED_DIGEST_HEX]", + "DIGEST": "_DEPRECATED_DIGEST_", + "DIGEST_ALGO": "_DEPRECATED_DIGEST_ALGO_", + "DIGEST_HEX": "_DEPRECATED_DIGEST_HEX_", }) if err != nil { return "", err } - if strings.Contains(tag, "[DEPRECATED_DIGEST]") || - strings.Contains(tag, "[DEPRECATED_DIGEST_ALGO]") || - strings.Contains(tag, "[DEPRECATED_DIGEST_HEX]") { - warner.Warnf("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags") + if strings.Contains(tag, "_DEPRECATED_DIGEST_") || + strings.Contains(tag, "_DEPRECATED_DIGEST_ALGO_") || + strings.Contains(tag, "_DEPRECATED_DIGEST_HEX_") { + warner.Warnf("{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags") switch { - case strings.HasSuffix(tag, "@[DEPRECATED_DIGEST]"): - tag = strings.TrimSuffix(tag, "@[DEPRECATED_DIGEST]") + case strings.HasSuffix(tag, "@_DEPRECATED_DIGEST_"): + tag = strings.TrimSuffix(tag, "@_DEPRECATED_DIGEST_") - case strings.HasSuffix(tag, "@[DEPRECATED_DIGEST_ALGO]:[DEPRECATED_DIGEST_HEX]"): - tag = strings.TrimSuffix(tag, "@[DEPRECATED_DIGEST_ALGO]:[DEPRECATED_DIGEST_HEX]") + case strings.HasSuffix(tag, "@_DEPRECATED_DIGEST_ALGO_:_DEPRECATED_DIGEST_HEX_"): + tag = strings.TrimSuffix(tag, "@_DEPRECATED_DIGEST_ALGO_:_DEPRECATED_DIGEST_HEX_") } } diff --git a/pkg/skaffold/build/tag/env_template_test.go b/pkg/skaffold/build/tag/env_template_test.go index fbeac1b1c01..49ccc8eee52 100644 --- a/pkg/skaffold/build/tag/env_template_test.go +++ b/pkg/skaffold/build/tag/env_template_test.go @@ -68,35 +68,35 @@ func TestEnvTemplateTagger_GenerateFullyQualifiedImageName(t *testing.T) { template: "{{.IMAGE_NAME}}:tag@{{.DIGEST}}", imageName: "foo", expected: "foo:tag", - expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags"}, }, { name: "ignore @{{.DIGEST_ALGO}}:{{.DIGEST_HEX}} suffix", template: "{{.IMAGE_NAME}}:tag@{{.DIGEST_ALGO}}:{{.DIGEST_HEX}}", imageName: "image_name", expected: "image_name:tag", - expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags"}, }, { name: "digest is deprecated", template: "{{.IMAGE_NAME}}:{{.DIGEST}}", imageName: "foo", - expected: "foo:[DEPRECATED_DIGEST]", - expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + expected: "foo:_DEPRECATED_DIGEST_", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags"}, }, { name: "digest algo is deprecated", template: "{{.IMAGE_NAME}}:{{.DIGEST_ALGO}}", imageName: "foo", - expected: "foo:[DEPRECATED_DIGEST_ALGO]", - expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + expected: "foo:_DEPRECATED_DIGEST_ALGO_", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags"}, }, { name: "digest hex is deprecated", template: "{{.IMAGE_NAME}}:{{.DIGEST_HEX}}", imageName: "foo", - expected: "foo:[DEPRECATED_DIGEST_HEX]", - expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be apppended to image tags"}, + expected: "foo:_DEPRECATED_DIGEST_HEX_", + expectedWarnings: []string{"{{.DIGEST}}, {{.DIGEST_ALGO}} and {{.DIGEST_HEX}} are deprecated, image digest will now automatically be appended to image tags"}, }, } for _, test := range tests {