Made build webhooks return new Build name. #12573

oatmealraisin · 2017-01-19T22:38:18Z

We now write the name of builds created with webhooks to the body of the
response, which was previously unused. This allows the client to alert
the user of the new build, similarly to if it was created through
non-webhook methods.

Old output:

$ oc start-build --from-webhook https://10.192.217.252:8443/oapi/v1/namespaces/myproject/buildconfigs/ruby-sample-build/webhooks/secret101/generic

New output:

$ oc start-build --from-webhook https://10.192.217.252:8443/oapi/v1/namespaces/myproject/buildconfigs/ruby-sample-build/webhooks/secret101/generic
build "ruby-sample-build-2" started

Fixes bz1373441

@csrwng @wanghaoran1988 ptal?

bparees · 2017-01-19T22:46:20Z

if the webhook is going to start returning a response body, then:

it's possible that's a breaking change, so we can't do it at all (@smarterclayton ?)
we need to define a struct for the body so we can extend our response object in the future, if we do what this PR proposes we're painted into a corner on extending the response body in the future.

bparees · 2017-01-19T22:47:04Z

also needs a test case

smarterclayton · 2017-01-19T23:24:45Z

We have to be careful about returning a body. But if we do, why wouldn't we return the build itself? On Jan 19, 2017, at 5:46 PM, Ben Parees <[email protected]> wrote: if the webhook is going to start returning a response body, then: 1. it's possible that's a breaking change, so we can't do it at all ( @smarterclayton <https://github.com/smarterclayton> ?) 2. we need to define a struct for the body so we can extend our response object in the future, if we do what this PR proposes we're painted into a corner on extending the response body in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p8FXYvF5w8fiHaCXBQQQjcZN5cLjks5rT-e9gaJpZM4Losf5> .

bparees · 2017-01-20T00:00:59Z

We have to be careful about returning a body.

can you be more explicit about what it means to be careful in this context?

But if we do, why wouldn't we return the build itself?

makes sense to me.

oatmealraisin · 2017-01-20T02:09:39Z

@smarterclayton @bparees We probably should be returning the build, I'll change that. However, I don't think this is a super important fix, because it just tells the user what the next build is. Up to you both.

bparees · 2017-01-20T03:08:17Z

I'm in support of it if we can get api sign off, but I agree the bug in question isn't critical. But the api would be better if it returned the object. Ben Parees | OpenShift

…

On Jan 19, 2017 21:09, "Ryan Murphy" ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> @bparees <https://github.com/bparees> We probably should be returning the build, I'll change that. However, I don't think this is a super important fix, because it just tells the user what the next build is. Up to you both. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3pSRdXxXNgphGQY9qL7SP4g8Dq9dks5rUBdogaJpZM4Losf5> .

oatmealraisin · 2017-01-20T15:59:26Z

Change it to returning the Build object, also added a test case to test-cmd

oatmealraisin · 2017-01-20T15:59:35Z

@bparees @smarterclayton shall we proceed?

bparees · 2017-01-20T16:02:02Z

pkg/build/registry/buildconfig/webhook.go

 		return errors.NewInternalError(fmt.Errorf("could not generate a build: %v", err))
 	}
+	// Send back the build name so that the client can alert the user.
+	newBuildJson, _ := json.Marshal(newBuild)


don't throw away errors. I think we should return an internal error, though that my be confusing since the build still got created. @openshift/api-review ?

I figured we wouldn't want to die if we got an error there. Maybe we should just print a warning and write an empty object?

it's not really "dying", because the build is going to get run regardless, it's about what you return to the caller.

Certainly just proceeding w/ writing the json seems like a mistake since you have no idea what's in there, given that the marshal returned an error.

bparees · 2017-01-20T16:03:55Z

@oatmealraisin aside from the marshal error it lgtm. Need to get @openshift/api-review opinion on how to handle the marshal error and general sign off on the change to return a body from this api.

deads2k · 2017-01-20T16:15:40Z

pkg/build/registry/buildconfig/webhook.go

 		return errors.NewInternalError(fmt.Errorf("could not generate a build: %v", err))
 	}
+	// Send back the build name so that the client can alert the user.
+	newBuildJson, _ := json.Marshal(newBuild)


Isn't this an internal version of the object? If so, then no client will be able to deserialize it. You'd need to go through a runtime.Encode path using a codec.

Also, if a caller is expecting a status object or a build object, writing something other than those things seems wrong (for the reasons @bparees already called out). Writing a status success (the call actually did succeed right?) that includes a message (intended to be human readable) that explains the situation seems more appropriate.

the caller is expecting nothing today. Returning a status object that includes the build (if nothing went wrong) sounds like the most flexible course of action.

the caller is expecting nothing today. Returning a status object that includes the build (if nothing went wrong) sounds like the most flexible course of action.

I intended to agree with what you said before about the encoding error case: you shouldn't return unknown content.

Returning a build object (as long as its an external version) seems reasonable for success cases. Returning a status object for encoding failure cases is ok. Most of our API already behaves that way.

I was trying to avoid an api that potentially returns two different types. The biggest challenge here is it's not exactly a "failure" case. The build was launched, but we can't return you the build object.

I was trying to avoid an api that potentially returns two different types. The biggest challenge here is it's not exactly a "failure" case. The build was launched, but we can't return you the build object.

That's already a thing. curl https://localhost:8443/api/v1/namespaces/foo if "foo" doesn't exist returns a kind: Status. curl -XDELETE https://localhost:8443/api/v1/namespaces/foo/something/bar has the option of returning either the resource you deleted or a status object on a 200 return.

ok cool, thanks for the sample. @oatmealraisin let's do that.

You can't use marshal here - you need to use the normal encoder (kapi.Codecs.LegacyCodec().Encode(...)

Oh, missed that David already said that in the thread.

Thanks all, I've changed it to use Encode()/Decode()

oatmealraisin · 2017-01-23T22:14:11Z

@bparees I've changed to the Encode/Decode logic, also now report all errors. ptal?

[test]

bparees · 2017-01-23T22:19:25Z

pkg/build/registry/buildconfig/webhook.go

+	// Send back the build name so that the client can alert the user.
+	newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(buildapi.SchemeGroupVersion), newBuild)
+	if err != nil {
+		return errors.NewInternalError(fmt.Errorf("could not encode build: %v", err))


error message should make it clear the build was still started/created.

Added a short string. Perhaps we should set warning to the err instead?

bparees · 2017-01-24T00:12:56Z

pkg/build/registry/buildconfig/webhook.go

+	// Send back the build name so that the client can alert the user.
+	newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(buildapi.SchemeGroupVersion), newBuild)
+	if err != nil {
+		return errors.NewInternalError(fmt.Errorf("Build %s was started, but could not encode build: %v", newBuild.Name, err))


"but could not encode response object"

lowercase B in build.

oatmealraisin · 2017-01-24T15:09:00Z

@bparees merge pending test?

bparees · 2017-01-24T15:16:04Z

@oatmealraisin awaiting @openshift/api-review sign off.

deads2k · 2017-01-24T18:13:00Z

pkg/build/registry/buildconfig/webhook.go

 		return errors.NewInternalError(fmt.Errorf("could not generate a build: %v", err))
 	}
+
+	// Send back the build name so that the client can alert the user.
+	newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(buildapi.SchemeGroupVersion), newBuild)


The version specified here looks wrong. You need to use the version under which you were requested so that a hook under /oapi/v1 returns Build.v1 and /oapi/v2 return Build.v2.

@deads2k Could you perhaps point me to an example of how to do this? Is there a util method?

@deads2k Could you perhaps point me to an example of how to do this? Is there a util method?

You could pass the groupversion to your RESTStorage and just hold it.

@deads2k Here's where it's created, where would I get the current version from? Not sure what I'm looking for.

@deads2k Here's where it's created, where would I get the current version from? Not sure what I'm looking for.

From that call site, you know it will be buildv1.SchemeGroupVersion. Whoever updates it to handle multiple versions (probably platform) will be able to follow your intent from there. Just pass that constant through. Make sure its v1.

@deads2k Roger, thanks!

deads2k · 2017-01-24T18:13:32Z

pkg/build/registry/buildconfig/webhook.go

+	// Send back the build name so that the client can alert the user.
+	newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(buildapi.SchemeGroupVersion), newBuild)
+	if err != nil {
+		return errors.NewInternalError(fmt.Errorf("build \"%s\" was started, but could not encode response object: %v", newBuild.Name, err))


This looks ok to me.

deads2k · 2017-01-24T18:15:21Z

@smarterclayton I like the idea, its predictable and consistent. I'll make sure it encodes correctly, but you happy with it otherwise?

deads2k · 2017-01-24T18:16:11Z

I'd like to see an integration test that actually ensures the wire encoding is correct.

smarterclayton · 2017-01-26T02:07:48Z

Barring future changes to support alternative protocols (which is unlikely), I agree with David. Marking as API approved.

oatmealraisin · 2017-01-26T20:08:18Z

@deads2k I've added a GroupVersion to the Webhook object that we get from master.go. I've also added a section in the Integration test to check that the returned build object can be decoded. ptal?

deads2k · 2017-01-27T15:49:06Z

pkg/cmd/cli/cmd/startbuild.go

+	body, _ := ioutil.ReadAll(resp.Body)
+
+	var newBuild buildapi.Build
+	if err = runtime.DecodeInto(kapi.Codecs.UniversalDecoder(), body, &newBuild); err != nil {


This breaks compatibilty with old servers since they don't return the build in the body. Also, be sure you properly handle status errors.

I believe status errors are handled shortly before this, are there some that would be able to make it to this part of the code?

deads2k · 2017-01-27T15:51:52Z

pkg/cmd/server/origin/master.go

@@ -656,6 +656,7 @@ func (c *MasterConfig) GetRestStorage() map[string]rest.Storage {
 	buildConfigWebHooks := buildconfigregistry.NewWebHookREST(
 		buildConfigRegistry,
 		buildclient.NewOSClientBuildConfigInstantiatorClient(bcClient),
+		v1.SchemeGroupVersion,


This should point explictly to the external group version from pkg/build/api/v1.

deads2k · 2017-01-27T15:54:14Z

Fix up the wiring to use the build specific constant and add a comment on it explaining what it's for and that it needs to match the the API version the storage is going to be used for.

After that, the version encoding looks right, so the API is fine. Pretty sure you broke client compatibility with old servers though and you can't do that.

oatmealraisin · 2017-01-30T19:26:17Z

@deads2k Thanks for the review, I've added a check in startbuild.go to make sure we don't break backwards compatibility. Also, I had to add a check in some webhook.go tests due to the error when runtime.Encode fails. ptal?

bparees

also needs a rebase.

bparees · 2017-02-17T18:13:21Z

pkg/build/registry/buildconfig/webhook.go

+	// Send back the build name so that the client can alert the user.
+	if newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(w.groupVersion), newBuild); err != nil {
+		glog.Warningf("build \"%s\" was started, but could not encode response object: %v", newBuild.Name, err)
+		//return errors.NewInternalError(fmt.Errorf("build \"%s\" was started, but could not encode response object: %v", newBuild.Name, err))


delete this.

bparees · 2017-02-17T18:18:24Z

test/integration/webhookgithub_test.go

@@ -299,6 +300,11 @@ func postFile(client restclient.HTTPClient, event, filename, url string, expStat
 	if resp.StatusCode != expStatusCode {
 		t.Errorf("Wrong response code, expecting %d, got %d: %s!", expStatusCode, resp.StatusCode, string(body))
 	}
+	var newBuild buildapi.Build
+	if err = runtime.DecodeInto(kapi.Codecs.UniversalDecoder(), body, &newBuild); err != nil {


bparees · 2017-02-17T18:18:32Z

test/cmd/builds.sh

@@ -97,7 +97,7 @@ os::cmd::expect_success_and_text 'oc start-build --list-webhooks=all bc/ruby-sam
 os::cmd::expect_success_and_text 'oc start-build --list-webhooks=all ruby-sample-build' 'github'
 os::cmd::expect_success_and_text 'oc start-build --list-webhooks=github ruby-sample-build' 'secret101'
 os::cmd::expect_failure 'oc start-build --list-webhooks=blah'
-os::cmd::expect_success "oc start-build --from-webhook='$(oc start-build --list-webhooks='generic' ruby-sample-build --api-version=v1 | head -n 1)'"
+os::cmd::expect_success_and_text "oc start-build --from-webhook='$(oc start-build --list-webhooks='generic' ruby-sample-build --api-version=v1 | head -n 1)'" "build \"ruby-sample-build-[0-9]\" started"


add a test the exercises the failure to instantiate flow.

I added a test that sends an odd webhook, which returns a 400 w/o a body. Is that what you meant?

i was thinking of a case where the build can't actually be started because, for example, the imagestream won't resolve.

(the test you added is good too)

@oatmealraisin i'm still looking for a test that validates the behavior when the build can't be started because it references an invalid/unresolved imagestream.

Sorry, I missed these comments. On it.

deads2k · 2017-02-23T13:52:23Z

pkg/build/registry/buildconfig/webhook.go

+
+	// Send back the build name so that the client can alert the user.
+	if newBuildEncoded, err := runtime.Encode(kapi.Codecs.LegacyCodec(w.groupVersion), newBuild); err != nil {
+		glog.Warningf("build \"%s\" was started, but could not encode response object: %v", newBuild.Name, err)


This is pretty bad. utilruntime.handleError so sentry smacks us and we go find you :)

Thanks for letting me know about that, I didn't know that existed :O Did you want this in addition to the warning, or instead of?

instead. handlerror does its own logging of the message so you don't need to.

oatmealraisin · 2017-03-01T21:58:45Z

@bparees I've added a few tests, I believe that should cover everything. ptal?

Sorry for the delay, I didn't know I needed to add _ "github.com/openshift/origin/pkg/api/install" to the tests to get Decode to work.

oatmealraisin · 2017-03-06T15:52:40Z

@bparees @deads2k bump

oatmealraisin · 2017-03-07T19:21:54Z

@bparees I added a test that patches ruby-sample-build to use a bad imagestream, and a test that runs that bc and captures the output. ptal?

oatmealraisin · 2017-03-07T19:22:23Z

@csrwng ptal?

oatmealraisin · 2017-03-08T21:46:37Z

@bparees @csrwng I changed a test to attempt to get the created build instead of using a watch.Interface. The watch was occasionally returning the wrong Build (build-1 instead of build-3). ptal?

bparees · 2017-03-09T03:11:33Z

test/integration/webhookgeneric_test.go

+		}
+
+		// TODO: improve negative testing
+		timer := time.NewTimer(time.Second / 2)


increase this timeout. 1/2 second is just begging for flakes.

Changed it to 5 minutes

bparees · 2017-03-09T03:19:17Z

The watch was occasionally returning the wrong Build (build-1 instead of build-3).

it would be good to understand why that is... assuming you create the watch before you create any builds, you the watch should always return the builds in the order they are created. (and the first build the watch should see looks like it would be one created by the imagechangetrigger, then you'd have 3 builds created by the github triggers).

bparees · 2017-03-09T03:22:03Z

i think the test was always flawed because it does the following:

create an imagestreammapping which will trigger a build
create a build watch
trigger more builds via the hook

1+2 are a race condition, the watch could include the build from (1) or it could not.

the easiest fix would be to create the buildconfig without an imagechangetrigger associated. then the only builds you'll get will be the hook triggered ones.

bparees · 2017-03-09T03:25:22Z

hm. nm, the buildconfig doesn't have an ICT on it. The reason you're seeing watch events for different builds is because builds go through multiple changes, so you get an event for each one and they could be interlaced. (build created, build pending, build running, etc). The build-1 event you saw when expecting a build-3 event was probably build-1 going to the pending state.

Elsewhere we've dealt with this by explicitly ignoring events for a build once we've seen the event we care about, but i think your approach is ok for the purposes of this test.

oatmealraisin · 2017-03-15T19:07:50Z

@bparees I've changed the timeout for the tests, ptal?

bparees · 2017-03-15T20:08:23Z

lgtm, squash and i'll merge.

bparees · 2017-03-16T00:42:52Z

[merge]

oatmealraisin · 2017-03-16T02:29:45Z

@bparees made the timer too long, shortening to 30sec, we'll see if that passes the test

bparees · 2017-03-16T02:36:09Z

If you're hitting the timeout, the test isn't working.... Ben Parees | OpenShift

…

On Mar 15, 2017 10:30 PM, "Ryan Murphy" ***@***.***> wrote: @bparees <https://github.com/bparees> made the timer too long, shortening to 30sec, we'll see if that passes the test — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3vesXFGL1coyBRlZPbxGMTqz9iJAks5rmJ6egaJpZM4Losf5> .

oatmealraisin · 2017-03-16T03:16:14Z

@bparees My mistake. I looked at the webhookgithub test first, where the timeout is the success case, and mirrored my changes to the generic test, where it is the opposite. I've changed the GitHub test back, and changed the bitbucket test to 3 minutes, which is under the 4 minute timeout that was getting triggered in the first place.

bparees · 2017-03-16T15:43:38Z

@bparees My mistake. I looked at the webhookgithub test first, where the timeout is the success case, and mirrored my changes to the generic test, where it is the opposite. I've changed the GitHub test back, and changed the bitbucket test to 3 minutes, which is under the 4 minute timeout that was getting triggered in the first place.

i think the webhookgithub timeout of 1/2 second is also too short.. It's so short that the test might pass simply because the "bad" event didn't occur yet. So that one should at least wait, say, 5 seconds, to ensure we are not going to get the event.
I'd reduce the 3 minute timeout to 1 minute, that's more than enough.

We now write the name of builds created with webhooks to the body of the response, which was previously unused. This allows the client to alert the user of the new build, similarly to if it was created through non-webhook methods. Added a GroupVersion parameter to Webhook object

oatmealraisin · 2017-03-16T19:04:47Z

@bparees Changed the timeouts, waiting on tests

oatmealraisin · 2017-03-17T19:48:10Z

Flake #12797, [test]

openshift-bot · 2017-03-17T19:53:17Z

Evaluated for origin test up to 5909f5f

bparees · 2017-03-17T19:54:33Z

[merge]

openshift-bot · 2017-03-17T20:13:17Z

Evaluated for origin merge up to 5909f5f

openshift-bot · 2017-03-17T20:13:27Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/129/) (Base Commit: e6a87ad) (Image: devenv-rhel7_6082)

openshift-bot · 2017-03-17T21:21:19Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/291/) (Base Commit: 9e19032)

bparees added the needs-api-review label Jan 19, 2017

bparees reviewed Jan 20, 2017

View reviewed changes

bparees self-assigned this Jan 20, 2017

deads2k reviewed Jan 20, 2017

View reviewed changes

bparees reviewed Jan 23, 2017

View reviewed changes

bparees reviewed Jan 24, 2017

View reviewed changes

deads2k reviewed Jan 24, 2017

View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels Jan 26, 2017

deads2k reviewed Jan 27, 2017

View reviewed changes

bparees requested changes Feb 17, 2017

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2017

deads2k reviewed Feb 23, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2017

bparees requested changes Mar 9, 2017

View reviewed changes

openshift-bot merged commit 769c079 into openshift:master Mar 17, 2017

Made build webhooks return new Build name. #12573

Made build webhooks return new Build name. #12573

Conversation

oatmealraisin commented Jan 19, 2017

bparees commented Jan 19, 2017

bparees commented Jan 19, 2017

smarterclayton commented Jan 19, 2017 via email

bparees commented Jan 20, 2017

oatmealraisin commented Jan 20, 2017

bparees commented Jan 20, 2017 via email

oatmealraisin commented Jan 20, 2017

oatmealraisin commented Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jan 20, 2017

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

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

oatmealraisin commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Jan 24, 2017

bparees commented Jan 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 24, 2017

deads2k commented Jan 24, 2017

smarterclayton commented Jan 26, 2017

oatmealraisin commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 27, 2017

oatmealraisin commented Jan 30, 2017

bparees left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Mar 1, 2017 • edited Loading

oatmealraisin commented Mar 6, 2017

oatmealraisin commented Mar 7, 2017

oatmealraisin commented Mar 7, 2017

oatmealraisin commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Mar 9, 2017

bparees commented Mar 9, 2017

bparees commented Mar 9, 2017

oatmealraisin commented Mar 15, 2017

bparees commented Mar 15, 2017

bparees commented Mar 16, 2017

oatmealraisin commented Mar 16, 2017

bparees commented Mar 16, 2017 via email

oatmealraisin commented Mar 16, 2017 • edited Loading

bparees commented Mar 16, 2017

oatmealraisin commented Mar 16, 2017

oatmealraisin commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

oatmealraisin commented Jan 20, 2017 •

edited

Loading

oatmealraisin commented Mar 1, 2017 •

edited

Loading

oatmealraisin commented Mar 16, 2017 •

edited

Loading

openshift-bot commented Mar 17, 2017 •

edited

Loading