Merge pull request #740 from dgageot/builder-config

dgageot · web-flow · commit 2e6ed08dc592 · 2018-06-25T13:37:18.000-07:00
Builders should only rely on their specific config
diff --git 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
@@ -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
@@ -34,31 +34,31 @@ 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
 	kubeContext  string
 }
 
 // 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.Wrapf(err, "pushing [%s]", tag)
 			}
diff --git 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
@@ -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)
