reject build requests for binary builds if not providing binary inputs

Author: bparees

pkg/build/generator/generator.go

@@ -218,9 +218,11 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe
 	if err != nil {
 		return nil, err
 	}
-
+	if request.Binary == nil && bc.Spec.Source.Dockerfile == nil && bc.Spec.Source.Git == nil && len(bc.Spec.Source.Images) == 0 {
+		return nil, errors.NewBadRequest(fmt.Sprintf("can't instantiate binary BuildConfig %s/%s: must provide a build input via --from-dir|--from-file|--from-repo", bc.Namespace, bc.Name))
+	}
 	if buildutil.IsPaused(bc) {
-		return nil, errors.NewInternalError(&GeneratorFatalError{fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", bc.Namespace, bc.Name)})
+		return nil, errors.NewBadRequest(fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", bc.Namespace, bc.Name))
 	}
 
 	if err := g.checkLastVersion(bc, request.LastVersion); err != nil {
@@ -333,6 +335,9 @@ func (g *BuildGenerator) Clone(ctx kapi.Context, request *buildapi.BuildRequest)
 		if err != nil && !errors.IsNotFound(err) {
 			return nil, err
 		}
+		if build.Spec.Source.Dockerfile == nil && build.Spec.Source.Git == nil && len(build.Spec.Source.Images) == 0 {
+			return nil, errors.NewBadRequest(fmt.Sprintf("can't clone binary Build %s/%s: start a new Build from the BuildConfig", build.Namespace, build.Name))
+		}
 
 		if buildutil.IsPaused(buildConfig) {
 			return nil, errors.NewInternalError(&GeneratorFatalError{fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", buildConfig.Namespace, buildConfig.Name)})

pkg/build/generator/generator_test.go

@@ -48,6 +48,35 @@ func TestInstantiate(t *testing.T) {
 	}
 }
 
+func TestInstantiateBinary(t *testing.T) {
+	generator := mockBuildGenerator()
+	/*
+		generator.Client.GetBuildConfigFunc = func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
+			bc := &buildapi.BuildConfig{}
+			return bc, nil
+		}
+		generator.Client.GetBuildFunc = func(ctx kapi.Context, name string) (*buildapi.Build, error) {
+			build := &buildapi.Build{
+				Status: buildapi.BuildStatus{
+					Config: &kapi.ObjectReference{
+						Name: "buildconfig",
+					},
+				},
+			}
+			return build, nil
+		}
+	*/
+	_, err := generator.Instantiate(kapi.NewDefaultContext(), &buildapi.BuildRequest{Binary: &buildapi.BinaryBuildSource{}})
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	// TODO: we should enable this flow.
+	_, err = generator.Clone(kapi.NewDefaultContext(), &buildapi.BuildRequest{Binary: &buildapi.BinaryBuildSource{}})
+	if err == nil || !strings.Contains(err.Error(), "can't clone binary Build") {
+		t.Errorf("Expected error, got different %v", err)
+	}
+}
+
 // TODO(agoldste): I'm not sure the intent of this test. Using the previous logic for
 // the generator, which would try to update the build config before creating
 // the build, I can see why the UpdateBuildConfigFunc is set up to return an
@@ -83,6 +112,7 @@ func TestInstantiateRetry(t *testing.T) {
 */
 
 func TestInstantiateDeletingError(t *testing.T) {
+	source := mocks.MockSource()
 	generator := BuildGenerator{Client: Client{
 		GetBuildConfigFunc: func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
 			bc := &buildapi.BuildConfig{
@@ -91,11 +121,31 @@ func TestInstantiateDeletingError(t *testing.T) {
 						buildapi.BuildConfigPausedAnnotation: "true",
 					},
 				},
+				Spec: buildapi.BuildConfigSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Source: source,
+						Revision: &buildapi.SourceRevision{
+							Git: &buildapi.GitSourceRevision{
+								Commit: "1234",
+							},
+						},
+					},
+				},
 			}
 			return bc, nil
 		},
 		GetBuildFunc: func(ctx kapi.Context, name string) (*buildapi.Build, error) {
 			build := &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Source: source,
+						Revision: &buildapi.SourceRevision{
+							Git: &buildapi.GitSourceRevision{
+								Commit: "1234",
+							},
+						},
+					},
+				},
 				Status: buildapi.BuildStatus{
 					Config: &kapi.ObjectReference{
 						Name: "buildconfig",
@@ -115,6 +165,33 @@ func TestInstantiateDeletingError(t *testing.T) {
 	}
 }
 
+func TestInstantiateBinaryError(t *testing.T) {
+	generator := BuildGenerator{Client: Client{
+		GetBuildConfigFunc: func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
+			bc := &buildapi.BuildConfig{}
+			return bc, nil
+		},
+		GetBuildFunc: func(ctx kapi.Context, name string) (*buildapi.Build, error) {
+			build := &buildapi.Build{
+				Status: buildapi.BuildStatus{
+					Config: &kapi.ObjectReference{
+						Name: "buildconfig",
+					},
+				},
+			}
+			return build, nil
+		},
+	}}
+	_, err := generator.Instantiate(kapi.NewDefaultContext(), &buildapi.BuildRequest{})
+	if err == nil || !strings.Contains(err.Error(), "can't instantiate binary BuildConfig") {
+		t.Errorf("Expected error, got different %v", err)
+	}
+	_, err = generator.Clone(kapi.NewDefaultContext(), &buildapi.BuildRequest{})
+	if err == nil || !strings.Contains(err.Error(), "can't clone binary Build") {
+		t.Errorf("Expected error, got different %v", err)
+	}
+}
+
 func TestInstantiateGetBuildConfigError(t *testing.T) {
 	generator := BuildGenerator{Client: Client{
 		GetBuildConfigFunc: func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
@@ -240,6 +317,7 @@ func TestInstantiateWithImageTrigger(t *testing.T) {
 		},
 	}
 
+	source := mocks.MockSource()
 	for _, tc := range tests {
 		bc := &buildapi.BuildConfig{
 			Spec: buildapi.BuildConfigSpec{
@@ -252,6 +330,12 @@ func TestInstantiateWithImageTrigger(t *testing.T) {
 							},
 						},
 					},
+					Source: source,
+					Revision: &buildapi.SourceRevision{
+						Git: &buildapi.GitSourceRevision{
+							Commit: "1234",
+						},
+					},
 				},
 				Triggers: tc.triggers,
 			},

pkg/cmd/cli/cmd/startbuild.go

@@ -165,6 +165,11 @@ func (o *StartBuildOptions) Complete(f *clientcmd.Factory, in io.Reader, out io.
 		return kcmdutil.UsageError(cmd, "Must pass a name of a build config or specify build name with '--from-build' flag")
 	}
 
+	if len(buildName) != 0 && (len(fromFile) != 0 || len(fromDir) != 0 || len(fromRepo) != 0) {
+		// TODO: we should support this, it should be possible to clone a build to run again with new uploaded artifacts.
+		// Doing so requires introducing a new clonebinary endpoint.
+		return kcmdutil.UsageError(cmd, "Cannot use '--from-build' flag with binary builds")
+	}
 	o.AsBinary = len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0
 
 	namespace, _, err := f.DefaultNamespace()