reject build requests for binary builds if not providing binary inputs #9679

bparees · 2016-07-01T21:56:50Z

new output:

$ oc start-build somebinarybuildcfg
Error from server: can't instantiate binary BuildConfig p1/somebinarybuildcfg: must provide a build input via --from-dir|--from-file|--from-repo

fixes #9261

bparees · 2016-07-01T23:07:58Z

@gabemontero ptal.

bparees · 2016-07-01T23:08:02Z

[test]

gabemontero · 2016-07-02T02:09:24Z

Igtm (if the test failure was from the changes I missed how the changes
would have caused the failure)

On Friday, July 1, 2016, Ben Parees [email protected] wrote:

@gabemontero https://github.com/gabemontero ptal.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9679 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ADbadFlS14GzYS_BWN8jSi_63P3KWKGoks5qRZ3VgaJpZM4JDghM
.

bparees · 2016-07-02T02:39:59Z

yeah appears to be a new flake, opened #9680

bparees · 2016-07-02T02:41:06Z

@smarterclayton can you ptal as well? i want to make sure i'm not missing something about the philosophy of binary builds.

smarterclayton · 2016-07-02T18:34:41Z

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 {


Why is this here and not in validation?

what validation? client-side we don't even retrieve the buildconfig that i saw, so we can't do it there.

and the buildrequest object validation also doesn't currently have a need to retrieve the buildconfig object in order to do validation, it seems burdensome to introduce that call in the validation flow.

(and the associated confusion of "what error do we return to a user when buildrequest validation failed because we got an spurious/temporary error retrieving the buildconfig associated with the buildrequest")

Validate the build you are creating

I think my approach allows for a nicer error message (in validation it's going to need to be a generic "everything is nil, you must specify one" message). but there's no doubt we were missing that validation, so i'll put it there.

go figure, it turns out we do have that validation, but binary is not remaining nil because we copy the value from the buildconfig when creating the build. will fix that.

bparees · 2016-07-02T21:32:22Z

@smarterclayton reworked, ptal. output is:

$ oc start-build centos
The Build "centos-2" is invalid.
spec.source: Invalid value: {"Binary":null,"Dockerfile":null,"Git":null,"Images":null,"ContextDir":"","SourceSecret":null,"Secrets":[]}: must provide a value for at least one source input(git, binary, dockerfile, images). For binary builds, use --from-dir|--from-repo|--from-file

and for --from-build:

$ oc start-build centos --from-build=centos-4
The Build "centos-5" is invalid.
spec.source: Invalid value: {"Binary":null,"Dockerfile":null,"Git":null,"Images":null,"ContextDir":"","SourceSecret":null,"Secrets":[]}: must provide a value for at least one source input(git, binary, dockerfile, images). For binary builds, use --from-dir|--from-repo|--from-file

bparees · 2016-07-02T21:34:02Z

and yeah that means oc create operations for builds are going to see:
must provide a value for at least one source input(git, binary, dockerfile, images). For binary builds, use --from-dir|--from-repo|--from-file

which doesn't make a ton of sense. which is one reason it was nicer to have an error coming out of the rest endpoint.

bparees · 2016-07-05T18:27:06Z

@smarterclayton bump

smarterclayton · 2016-07-05T18:40:18Z

pkg/build/api/validation/validation.go

@@ -142,7 +142,7 @@ func validateCommonSpec(spec *buildapi.CommonSpec, fldPath *field.Path) field.Er
 	s := spec.Strategy

 	if s.DockerStrategy != nil && s.JenkinsPipelineStrategy == nil && spec.Source.Git == nil && spec.Source.Binary == nil && spec.Source.Dockerfile == nil && spec.Source.Images == nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("source"), spec.Source, "must provide a value for at least one of source, binary, images, or dockerfile"))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("source"), spec.Source, "must provide a value for at least one source input(git, binary, dockerfile, images). For binary builds, use --from-dir|--from-repo|--from-file"))


We shouldn't be sending command line instructions back from the API

bparees · 2016-07-06T02:35:34Z

[testextended][extended:core(starting a build using CLI)]

bparees · 2016-07-06T02:43:13Z

@smarterclayton i am not doing checking inside the instantiate/clone code because at that point we're getting back a StatusError from the create call, which means instantiate/clone needs to:

cast the error to a statuserror
unwrap the validation error within the status error (if it's even still present in its original form, i didn't even get that far before getting frustrated w/ it being a fiddly mess)
check if the validation error pertains to spec.source and appears to indicate a binary build related problem
return a bad request error to the instantiate/clone caller

then the instantiate/clone caller (client in this case) has to interpret the bad request error again, to present a useful error message to the user.

imho the validation error being returned directly from the instantiate/clone api is sufficient for a client to do the right thing. They tried to instantiate a build, they got a validation error.

bparees · 2016-07-07T18:24:43Z

@smarterclayton bump.

smarterclayton · 2016-07-11T02:36:22Z

pkg/build/generator/generator.go

 	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))


We should be setting a cause on errors like this - can you open an issue?

what, against upstream?

bparees · 2016-07-11T02:56:34Z

pkg/cmd/cli/cmd/startbuild.go

+	if err != nil {
+		if statusErr, ok := err.(*kerrors.StatusError); ok {
+			if statusErr.ErrStatus.Reason == "Invalid" {
+				for _, cause := range statusErr.ErrStatus.Details.Causes {


@smarterclayton better? doesn't seem better.

No, just use the source field "spec.source" instead of the message (you get that separate from message). And don't check Reason against string "Invalid", check the error type against api/errors.IsInvalid

bparees · 2016-07-11T03:13:59Z

pkg/cmd/cli/cmd/startbuild.go

+		if statusErr, ok := err.(*kerrors.StatusError); ok {
+			if kerrors.IsInvalid(statusErr) {
+				for _, cause := range statusErr.ErrStatus.Details.Causes {
+					if cause.Field == "spec.source" {


@smarterclayton how about now?

smarterclayton · 2016-07-11T22:28:52Z

LGTM

openshift-bot · 2016-07-11T22:50:42Z

Evaluated for origin testextended up to bc80045

openshift-bot · 2016-07-11T23:25:48Z

Evaluated for origin test up to bc80045

openshift-bot · 2016-07-11T23:30:49Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/317/) (Extended Tests: core(starting a build using CLI))

openshift-bot · 2016-07-12T00:41:16Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6084/)

bparees · 2016-07-12T02:52:51Z

[merge]

openshift-bot · 2016-07-12T02:55:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6088/) (Image: devenv-rhel7_4572)

openshift-bot · 2016-07-12T02:55:28Z

Evaluated for origin merge up to bc80045

bparees force-pushed the binary_build_error branch 3 times, most recently from 1587b5d to 868b80b Compare July 1, 2016 23:07

bparees force-pushed the binary_build_error branch from 868b80b to 1dcb64c Compare July 2, 2016 01:01

smarterclayton reviewed Jul 2, 2016
View reviewed changes

bparees force-pushed the binary_build_error branch from 1dcb64c to c5107ed Compare July 2, 2016 21:29

smarterclayton reviewed Jul 5, 2016
View reviewed changes

bparees force-pushed the binary_build_error branch 2 times, most recently from eb92fd9 to fcac803 Compare July 6, 2016 02:35

smarterclayton reviewed Jul 11, 2016
View reviewed changes

bparees force-pushed the binary_build_error branch 2 times, most recently from d9cd307 to b5bc3fd Compare July 11, 2016 02:56

bparees reviewed Jul 11, 2016
View reviewed changes

bparees force-pushed the binary_build_error branch from b5bc3fd to 183e5b0 Compare July 11, 2016 03:10

bparees reviewed Jul 11, 2016
View reviewed changes

bparees force-pushed the binary_build_error branch 2 times, most recently from 049712a to 40bd5b6 Compare July 11, 2016 19:23

bparees force-pushed the binary_build_error branch from 40bd5b6 to b13da08 Compare July 11, 2016 21:05

reject build requests for binary builds if not providing binary inputs

bc80045

bparees force-pushed the binary_build_error branch from b13da08 to bc80045 Compare July 11, 2016 22:48

openshift-bot merged commit 522a826 into openshift:master Jul 12, 2016

bparees deleted the binary_build_error branch July 13, 2016 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reject build requests for binary builds if not providing binary inputs #9679

reject build requests for binary builds if not providing binary inputs #9679

bparees commented Jul 1, 2016 •

edited

Loading

bparees commented Jul 1, 2016

bparees commented Jul 1, 2016

gabemontero commented Jul 2, 2016

bparees commented Jul 2, 2016

bparees commented Jul 2, 2016

smarterclayton Jul 2, 2016

bparees Jul 2, 2016

bparees Jul 2, 2016

smarterclayton Jul 2, 2016 via email

bparees Jul 2, 2016

bparees Jul 2, 2016

bparees commented Jul 2, 2016

bparees commented Jul 2, 2016

bparees commented Jul 5, 2016

smarterclayton Jul 5, 2016

bparees commented Jul 6, 2016

bparees commented Jul 6, 2016

bparees commented Jul 7, 2016

smarterclayton Jul 11, 2016

bparees Jul 11, 2016

bparees Jul 11, 2016

smarterclayton Jul 11, 2016

bparees Jul 11, 2016

smarterclayton commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 12, 2016

bparees commented Jul 12, 2016

openshift-bot commented Jul 12, 2016 •

edited

Loading

openshift-bot commented Jul 12, 2016

reject build requests for binary builds if not providing binary inputs #9679

reject build requests for binary builds if not providing binary inputs #9679

Conversation

bparees commented Jul 1, 2016 • edited Loading

bparees commented Jul 1, 2016

bparees commented Jul 1, 2016

gabemontero commented Jul 2, 2016

bparees commented Jul 2, 2016

bparees commented Jul 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton Jul 2, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 2, 2016

bparees commented Jul 2, 2016

bparees commented Jul 5, 2016

Choose a reason for hiding this comment

bparees commented Jul 6, 2016

bparees commented Jul 6, 2016

bparees commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

openshift-bot commented Jul 12, 2016

bparees commented Jul 12, 2016

openshift-bot commented Jul 12, 2016 • edited Loading

openshift-bot commented Jul 12, 2016

bparees commented Jul 1, 2016 •

edited

Loading

openshift-bot commented Jul 12, 2016 •

edited

Loading