From df8ed7c8b365794c2252bb503129f45c1c90e71e Mon Sep 17 00:00:00 2001 From: David Gageot Date: Sun, 24 Jun 2018 19:49:39 +0200 Subject: [PATCH] Builders should only rely on their specific config --- pkg/skaffold/build/container_builder.go | 22 +++-- pkg/skaffold/build/kaniko.go | 18 ++-- pkg/skaffold/build/local.go | 12 +-- pkg/skaffold/build/local_test.go | 118 ++++++------------------ pkg/skaffold/runner/runner.go | 6 +- 5 files changed, 58 insertions(+), 118 deletions(-) diff --git a/pkg/skaffold/build/container_builder.go b/pkg/skaffold/build/container_builder.go index 7c8400ab377..1fed7e5ddf7 100644 --- a/pkg/skaffold/build/container_builder.go +++ b/pkg/skaffold/build/container_builder.go @@ -68,11 +68,13 @@ const ( ) type GoogleCloudBuilder struct { - *v1alpha2.BuildConfig + *v1alpha2.GoogleCloudBuild } -func NewGoogleCloudBuilder(cfg *v1alpha2.BuildConfig) (*GoogleCloudBuilder, error) { - return &GoogleCloudBuilder{BuildConfig: cfg}, nil +func NewGoogleCloudBuilder(cfg *v1alpha2.GoogleCloudBuild) (*GoogleCloudBuilder, error) { + return &GoogleCloudBuilder{ + GoogleCloudBuild: cfg, + }, nil } func (cb *GoogleCloudBuilder) Labels() map[string]string { @@ -124,8 +126,8 @@ func (cb *GoogleCloudBuilder) buildArtifact(ctx context.Context, out io.Writer, } logrus.Debugf("Build args: %s", buildArgs) - cbBucket := fmt.Sprintf("%s%s", cb.GoogleCloudBuild.ProjectID, constants.GCSBucketSuffix) - buildObject := fmt.Sprintf("source/%s-%s.tar.gz", cb.GoogleCloudBuild.ProjectID, util.RandomID()) + cbBucket := fmt.Sprintf("%s%s", cb.ProjectID, constants.GCSBucketSuffix) + buildObject := fmt.Sprintf("source/%s-%s.tar.gz", cb.ProjectID, util.RandomID()) if err := cb.createBucketIfNotExists(ctx, cbBucket); err != nil { return nil, errors.Wrap(err, "creating bucket if not exists") @@ -141,7 +143,7 @@ func (cb *GoogleCloudBuilder) buildArtifact(ctx context.Context, out io.Writer, args := append([]string{"build", "--tag", artifact.ImageName, "-f", artifact.DockerArtifact.DockerfilePath}, buildArgs...) args = append(args, ".") - call := cbclient.Projects.Builds.Create(cb.GoogleCloudBuild.ProjectID, &cloudbuild.Build{ + call := cbclient.Projects.Builds.Create(cb.ProjectID, &cloudbuild.Build{ LogsBucket: cbBucket, Source: &cloudbuild.Source{ StorageSource: &cloudbuild.StorageSource{ @@ -173,7 +175,7 @@ func (cb *GoogleCloudBuilder) buildArtifact(ctx context.Context, out io.Writer, watch: for { logrus.Debugf("current offset %d", offset) - b, err := cbclient.Projects.Builds.Get(cb.GoogleCloudBuild.ProjectID, remoteID).Do() + b, err := cbclient.Projects.Builds.Get(cb.ProjectID, remoteID).Do() if err != nil { return nil, errors.Wrap(err, "getting build status") } @@ -284,7 +286,7 @@ func (cb *GoogleCloudBuilder) checkBucketProjectCorrect(ctx context.Context, buc if err != nil { return errors.Wrap(err, "getting storage client") } - it := c.Buckets(ctx, cb.GoogleCloudBuild.ProjectID) + it := c.Buckets(ctx, cb.ProjectID) // Set the prefix to the bucket we're looking for to only return that bucket and buckets with that prefix // that we'll filter further later on it.Prefix = bucket @@ -322,11 +324,11 @@ func (cb *GoogleCloudBuilder) createBucketIfNotExists(ctx context.Context, bucke return errors.Wrapf(err, "getting bucket %s", bucket) } - if err := c.Bucket(bucket).Create(ctx, cb.GoogleCloudBuild.ProjectID, &cstorage.BucketAttrs{ + if err := c.Bucket(bucket).Create(ctx, cb.ProjectID, &cstorage.BucketAttrs{ Name: bucket, }); err != nil { return err } - logrus.Debugf("Created bucket %s in %s", bucket, cb.GoogleCloudBuild.ProjectID) + logrus.Debugf("Created bucket %s in %s", bucket, cb.ProjectID) return nil } diff --git a/pkg/skaffold/build/kaniko.go b/pkg/skaffold/build/kaniko.go index 33b73f60a66..087a53f4d71 100644 --- a/pkg/skaffold/build/kaniko.go +++ b/pkg/skaffold/build/kaniko.go @@ -36,13 +36,13 @@ import ( // KanikoBuilder can build docker artifacts on Kubernetes, using Kaniko. type KanikoBuilder struct { - *v1alpha2.BuildConfig + *v1alpha2.KanikoBuild } // NewKanikoBuilder creates a KanikoBuilder. -func NewKanikoBuilder(cfg *v1alpha2.BuildConfig) (*KanikoBuilder, error) { +func NewKanikoBuilder(cfg *v1alpha2.KanikoBuild) (*KanikoBuilder, error) { return &KanikoBuilder{ - BuildConfig: cfg, + KanikoBuild: cfg, }, nil } @@ -104,26 +104,26 @@ func (k *KanikoBuilder) setupSecret() (func(), error) { return nil, errors.Wrap(err, "getting kubernetes client") } - secrets := client.CoreV1().Secrets(k.KanikoBuild.Namespace) + secrets := client.CoreV1().Secrets(k.Namespace) - if k.KanikoBuild.PullSecret == "" { + if k.PullSecret == "" { logrus.Debug("No pull secret specified. Checking for one in the cluster.") - if _, err := secrets.Get(k.KanikoBuild.PullSecretName, metav1.GetOptions{}); err != nil { + if _, err := secrets.Get(k.PullSecretName, metav1.GetOptions{}); err != nil { return nil, errors.Wrap(err, "checking for existing kaniko secret") } return func() {}, nil } - secretData, err := ioutil.ReadFile(k.KanikoBuild.PullSecret) + secretData, err := ioutil.ReadFile(k.PullSecret) if err != nil { return nil, errors.Wrap(err, "reading secret") } secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: k.KanikoBuild.PullSecretName, + Name: k.PullSecretName, Labels: map[string]string{"skaffold-kaniko": "skaffold-kaniko"}, }, Data: map[string][]byte{ @@ -136,7 +136,7 @@ func (k *KanikoBuilder) setupSecret() (func(), error) { } return func() { - if err := secrets.Delete(k.KanikoBuild.PullSecretName, &metav1.DeleteOptions{}); err != nil { + if err := secrets.Delete(k.PullSecretName, &metav1.DeleteOptions{}); err != nil { logrus.Warnf("deleting secret") } }, nil diff --git a/pkg/skaffold/build/local.go b/pkg/skaffold/build/local.go index b2d071bef39..6cf46231a84 100644 --- a/pkg/skaffold/build/local.go +++ b/pkg/skaffold/build/local.go @@ -34,7 +34,7 @@ import ( // LocalBuilder uses the host docker daemon to build and tag the image type LocalBuilder struct { - *v1alpha2.BuildConfig + *v1alpha2.LocalBuild api docker.APIClient localCluster bool @@ -42,23 +42,23 @@ type LocalBuilder struct { } // NewLocalBuilder returns an new instance of a LocalBuilder -func NewLocalBuilder(cfg *v1alpha2.BuildConfig, kubeContext string) (*LocalBuilder, error) { +func NewLocalBuilder(cfg *v1alpha2.LocalBuild, kubeContext string) (*LocalBuilder, error) { api, err := docker.NewAPIClient() if err != nil { return nil, errors.Wrap(err, "getting docker client") } l := &LocalBuilder{ - BuildConfig: cfg, + LocalBuild: cfg, kubeContext: kubeContext, api: api, localCluster: kubeContext == constants.DefaultMinikubeContext || kubeContext == constants.DefaultDockerForDesktopContext, } - if cfg.LocalBuild.SkipPush == nil { + if cfg.SkipPush == nil { logrus.Debugf("skipPush value not present. defaulting to cluster default %t (minikube=true, d4d=true, gke=false)", l.localCluster) - cfg.LocalBuild.SkipPush = &l.localCluster + cfg.SkipPush = &l.localCluster } return l, nil @@ -126,7 +126,7 @@ func (l *LocalBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagg if _, err := io.WriteString(out, fmt.Sprintf("Successfully tagged %s\n", tag)); err != nil { return nil, errors.Wrap(err, "writing tag status") } - if !*l.LocalBuild.SkipPush { + if !*l.SkipPush { if err := docker.RunPush(ctx, l.api, tag, out); err != nil { return nil, errors.Wrap(err, "running push") } diff --git a/pkg/skaffold/build/local_test.go b/pkg/skaffold/build/local_test.go index 1a154147616..467e498a88d 100644 --- a/pkg/skaffold/build/local_test.go +++ b/pkg/skaffold/build/local_test.go @@ -68,7 +68,7 @@ func TestLocalRun(t *testing.T) { var tests = []struct { description string - config *v1alpha2.BuildConfig + config *v1alpha2.LocalBuild out io.Writer api docker.APIClient tagger tag.Tagger @@ -80,19 +80,15 @@ func TestLocalRun(t *testing.T) { { description: "single build", out: ioutil.Discard, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "gcr.io/test/image", - Workspace: tmp, - ArtifactType: v1alpha2.ArtifactType{ - DockerArtifact: &v1alpha2.DockerArtifact{}, - }, - }, - }, - BuildType: v1alpha2.BuildType{ - LocalBuild: &v1alpha2.LocalBuild{ - SkipPush: util.BoolPtr(false), + config: &v1alpha2.LocalBuild{ + SkipPush: util.BoolPtr(false), + }, + artifacts: []*v1alpha2.Artifact{ + { + ImageName: "gcr.io/test/image", + Workspace: tmp, + ArtifactType: v1alpha2.ArtifactType{ + DockerArtifact: &v1alpha2.DockerArtifact{}, }, }, }, @@ -108,28 +104,8 @@ func TestLocalRun(t *testing.T) { { description: "subset build", out: ioutil.Discard, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "gcr.io/test/image", - Workspace: tmp, - ArtifactType: v1alpha2.ArtifactType{ - DockerArtifact: &v1alpha2.DockerArtifact{}, - }, - }, - { - ImageName: "gcr.io/test/image2", - Workspace: tmp, - ArtifactType: v1alpha2.ArtifactType{ - DockerArtifact: &v1alpha2.DockerArtifact{}, - }, - }, - }, - BuildType: v1alpha2.BuildType{ - LocalBuild: &v1alpha2.LocalBuild{ - SkipPush: util.BoolPtr(true), - }, - }, + config: &v1alpha2.LocalBuild{ + SkipPush: util.BoolPtr(true), }, tagger: &tag.ChecksumTagger{}, artifacts: []*v1alpha2.Artifact{ @@ -152,22 +128,15 @@ func TestLocalRun(t *testing.T) { { description: "local cluster bad writer", out: &testutil.BadWriter{}, - config: &v1alpha2.BuildConfig{}, + config: &v1alpha2.LocalBuild{}, shouldErr: true, localCluster: true, }, { description: "error image build", out: ioutil.Discard, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "test", - Workspace: ".", - }, - }, - }, - tagger: &tag.ChecksumTagger{}, + artifacts: []*v1alpha2.Artifact{{}}, + tagger: &tag.ChecksumTagger{}, api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{ ErrImageBuild: true, }), @@ -176,15 +145,8 @@ func TestLocalRun(t *testing.T) { { description: "error image tag", out: ioutil.Discard, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "test", - Workspace: ".", - }, - }, - }, - tagger: &tag.ChecksumTagger{}, + artifacts: []*v1alpha2.Artifact{{}}, + tagger: &tag.ChecksumTagger{}, api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{ ErrImageTag: true, }), @@ -193,30 +155,16 @@ func TestLocalRun(t *testing.T) { { description: "bad writer", out: &testutil.BadWriter{}, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "test", - Workspace: ".", - }, - }, - }, - tagger: &tag.ChecksumTagger{}, - api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}), - shouldErr: true, + artifacts: []*v1alpha2.Artifact{{}}, + tagger: &tag.ChecksumTagger{}, + api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}), + shouldErr: true, }, { description: "error image list", out: &testutil.BadWriter{}, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "test", - Workspace: ".", - }, - }, - }, - tagger: &tag.ChecksumTagger{}, + artifacts: []*v1alpha2.Artifact{{}}, + tagger: &tag.ChecksumTagger{}, api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{ ErrImageList: true, }), @@ -225,30 +173,20 @@ func TestLocalRun(t *testing.T) { { description: "error tagger", out: ioutil.Discard, - config: &v1alpha2.BuildConfig{ - Artifacts: []*v1alpha2.Artifact{ - { - ImageName: "test", - Workspace: ".", - }, - }, - }, - tagger: &FakeTagger{Err: fmt.Errorf("")}, - api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}), - shouldErr: true, + artifacts: []*v1alpha2.Artifact{{}}, + tagger: &FakeTagger{Err: fmt.Errorf("")}, + api: testutil.NewFakeImageAPIClient(map[string]string{}, &testutil.FakeImageAPIOptions{}), + shouldErr: true, }, } for _, test := range tests { t.Run(test.description, func(t *testing.T) { l := LocalBuilder{ - BuildConfig: test.config, + LocalBuild: test.config, api: test.api, localCluster: test.localCluster, } - if test.artifacts == nil { - test.artifacts = test.config.Artifacts - } res, err := l.Build(context.Background(), test.out, test.tagger, test.artifacts) testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, res) diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index 0a7d9baa947..c041ce6f633 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -89,15 +89,15 @@ func getBuilder(cfg *v1alpha2.BuildConfig, kubeContext string) (build.Builder, e switch { case cfg.LocalBuild != nil: logrus.Debugf("Using builder: local") - return build.NewLocalBuilder(cfg, kubeContext) + return build.NewLocalBuilder(cfg.LocalBuild, kubeContext) case cfg.GoogleCloudBuild != nil: logrus.Debugf("Using builder: google cloud") - return build.NewGoogleCloudBuilder(cfg) + return build.NewGoogleCloudBuilder(cfg.GoogleCloudBuild) case cfg.KanikoBuild != nil: logrus.Debugf("Using builder: kaniko") - return build.NewKanikoBuilder(cfg) + return build.NewKanikoBuilder(cfg.KanikoBuild) default: return nil, fmt.Errorf("Unknown builder for config %+v", cfg)