support for gitlab and bitbucket webhooks #13389

gabemontero · 2017-03-14T21:24:49Z

@openshift/devex PTAL

per the discussion during sprint planning, we did not updatea the --from-webhook, etc. options with oc start-build, nor did we change oc new-app to either automatically add the triggers for gitlab and webhook or add yet another option to oc new-app to let the user choose to automatically add the triggers.

Of course we can further debate those points along with the content of the code changes as everyone sees fit.

coreydaley · 2017-03-15T14:14:28Z

pkg/build/api/types.go

+	BuildTriggerCauseGithubMsg    = "GitHub WebHook"
+	BuildTriggerCauseGenericMsg   = "Generic WebHook"
+	BuildTriggerCauseGitlabMsg    = "Gitlab WebHook"
+	BuildTriggerCauseBitBucketMsg = "BitBucket WebHook"


Just a nit, it's Bitbucket not BitBucket (also other places in the pr)

coreydaley · 2017-03-15T14:16:50Z

pkg/build/api/types.go

+	BuildTriggerCauseImageMsg     = "Image change"
+	BuildTriggerCauseGithubMsg    = "GitHub WebHook"
+	BuildTriggerCauseGenericMsg   = "Generic WebHook"
+	BuildTriggerCauseGitlabMsg    = "Gitlab WebHook"


nit: should be GitLab (consistent with other places in the PR)

coreydaley · 2017-03-15T14:28:55Z

pkg/build/api/v1/types.go

+
+	// GitLabWebHook represents data for a GitLab webhook that fired a specific
+	// build.
+	GitLabWebHook *GitLabWebHookCause `protobuf:"bytes,5,opt,name=gitLabWebHook"`


missing json section

coreydaley · 2017-03-15T14:29:05Z

pkg/build/api/v1/types.go

+
+	// BitBucketWebHook represents data for a BitBucket webhook that fired a
+	// specific build.
+	BitBucketWebHook *BitBucketWebHookCause `protobuf:"bytes,6,opt,name=bitBucketWebHook"`


missing json section

coreydaley · 2017-03-15T14:29:38Z

pkg/build/api/v1/types.go

+// build.
+type GitLabWebHookCause struct {
+	// Revision is the git source revision information of the trigger.
+	Revision *SourceRevision `protobuf:"bytes,1,opt,name=revision"`


missing json section

coreydaley · 2017-03-15T14:29:46Z

pkg/build/api/v1/types.go

+	Revision *SourceRevision `protobuf:"bytes,1,opt,name=revision"`
+
+	// Secret is the obfuscated webhook secret that triggered a build.
+	Secret string `protobuf:"bytes,2,opt,name=secret"`


missing json section

coreydaley · 2017-03-15T14:29:53Z

pkg/build/api/v1/types.go

+// build.
+type BitBucketWebHookCause struct {
+	// Revision is the git source revision information of the trigger.
+	Revision *SourceRevision `protobuf:"bytes,1,opt,name=revision"`


missing json section

coreydaley · 2017-03-15T14:30:00Z

pkg/build/api/v1/types.go

+	Revision *SourceRevision `protobuf:"bytes,1,opt,name=revision"`
+
+	// Secret is the obfuscated webhook secret that triggered a build.
+	Secret string `protobuf:"bytes,2,opt,name=secret"`


missing json section

coreydaley · 2017-03-15T14:30:08Z

pkg/build/api/v1/types.go

@@ -780,6 +808,13 @@ type BuildTriggerPolicy struct {

 	// imageChange contains parameters for an ImageChange type of trigger
 	ImageChange *ImageChangeTrigger `json:"imageChange,omitempty" protobuf:"bytes,4,opt,name=imageChange"`
+
+	// GitLabWebHook contains the parameters for a GitLab webhook type of trigger
+	GitLabWebHook *WebHookTrigger `protobuf:"bytes,5,opt,name=gitLabWebHook"`


missing json section

coreydaley · 2017-03-15T14:30:19Z

pkg/build/api/v1/types.go

+
+	// BitBucketWebHook contains the parameters for a BitBucket webhook type of
+	// trigger
+	BitBucketWebHook *WebHookTrigger `protobuf:"bytes,6,opt,name=bitBucketWebHook"`


missing json section

coreydaley · 2017-03-15T14:36:59Z

pkg/build/webhook/bitbucket/bitbucket.go

+	Name string `json:"name"`
+}
+
+// Extract services webhooks from github.com


should be bitbucket

coreydaley · 2017-03-15T14:39:59Z

pkg/build/webhook/gitlab/gitlab.go

+}
+
+// NOTE - unlike github, the head commit is not highlighted ... only the commit array is provided,
+// where it the last commit is the latest commit


coreydaley · 2017-03-15T14:40:16Z

pkg/build/webhook/gitlab/gitlab.go

+	Commits []commit `json:"commits,omitempty"`
+}
+
+// Extract services webhooks from github.com


should be gitlab

coreydaley · 2017-03-15T14:42:44Z

pkg/build/webhook/gitlab/gitlab_test.go

+		CommonSpec: api.CommonSpec{
+			Source: api.BuildSource{
+				Git: &api.GitBuildSource{
+					URI: "git://github.com/my/repo.git",


should be gitlab.com?

I think this can be any URI? I think the intended use is a custom gitlab server.

Sounds good

if you want to make it clearer you can make it git://someurl.com/my/repo.git

coreydaley · 2017-03-15T14:45:18Z

pkg/build/webhook/gitlab/gitlab_test.go

+				CommonSpec: api.CommonSpec{
+					Source: api.BuildSource{
+						Git: &api.GitBuildSource{
+							URI: "git://github.com/my/repo.git",


should be gitlab.com?

I think this can be any URI? I think the intended use is a custom gitlab server.

Sounds good

coreydaley · 2017-03-15T14:46:34Z

pkg/build/webhook/webhook.go

@@ -61,6 +61,7 @@ func FindTriggerPolicy(triggerType buildapi.BuildTriggerType, config *buildapi.B
 // defined webhook secrets and if it is valid, returns its information.
 func ValidateWebHookSecret(webHookTriggers []buildapi.BuildTriggerPolicy, secret string) (*buildapi.WebHookTrigger, error) {
 	for _, trigger := range webHookTriggers {
+		//TODO Could we switch this to a switch?


agree, might be cleaner as a switch

coreydaley · 2017-03-15T14:49:50Z

test/integration/webhookgithub_test.go

@@ -327,6 +327,42 @@ func mockBuildConfigImageParms(imageName, imageStream, imageTag string) *buildap
 						Secret: "secret102",
 					},
 				},
+				{
+					Type: buildapi.GitLabWebHookBuildTriggerType,
+					GitLabWebHook: &buildapi.WebHookTrigger{


maybe change the name of this file if it is going to include gitlab and bitbucket tests also?

I definitely agree. In another PR, I had to make a webhookgeneric_test, which doesn't look good. On it.

gabemontero · 2017-03-15T15:47:23Z

fyi @oatmealraisin is taking on the first set of comments here from @coreydaley ... he'll push / squash when complete

smarterclayton · 2017-03-15T15:57:55Z

Did you update set triggers?

gabemontero · 2017-03-15T16:04:16Z

On Wed, Mar 15, 2017 at 11:58 AM, Clayton Coleman ***@***.***> wrote: Did you update set triggers?

We explicitly did not do that per interpretation of discussion during sprint planning with @bparees. That discussion centered around ` --from-webhook` and `--git-post-receive` from `oc start-build` But perhaps we confused things wrt not doing `oc set triggers` (which also has a --from-webhook as well) ?

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADbadCQp-BGtNrnzGEBtpZMNXL7b7g97ks5rmAqJgaJpZM4MdLKN> .

smarterclayton · 2017-03-15T16:26:27Z

I'm ok with not expanding new-build or new-app or start-build, but you need to manage with "set triggers". Otherwise it's not possible from the CLI On Wed, Mar 15, 2017 at 12:04 PM, Gabe Montero <[email protected]> wrote:

…

On Wed, Mar 15, 2017 at 11:58 AM, Clayton Coleman < ***@***.***> wrote: > Did you update set triggers? > We explicitly did not do that per interpretation of discussion during sprint planning with @bparees. That discussion centered around ` --from-webhook` and `--git-post-receive` from `oc start-build` But perhaps we confused things wrt not doing `oc set triggers` (which also has a --from-webhook as well) ? > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#13389 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ADbadCQp- BGtNrnzGEBtpZMNXL7b7g97ks5rmAqJgaJpZM4MdLKN> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pzjZf2BrAd2kjnK4AZk_itAmvUWrks5rmAwCgaJpZM4MdLKN> .

gabemontero · 2017-03-15T16:53:24Z

On Wed, Mar 15, 2017 at 12:26 PM, Clayton Coleman <[email protected]> wrote:

I'm ok with not expanding new-build or new-app or start-build, but you need to manage with "set triggers". Otherwise it's not possible from the CLI

10-4 we'll update

…

On Wed, Mar 15, 2017 at 12:04 PM, Gabe Montero ***@***.***> wrote: > On Wed, Mar 15, 2017 at 11:58 AM, Clayton Coleman < > ***@***.***> > wrote: > > > Did you update set triggers? > > > We explicitly did not do that per interpretation of discussion during > sprint planning with @bparees. > > That discussion centered around ` --from-webhook` and `--git-post-receive` > from `oc start-build` > > But perhaps we confused things wrt not doing `oc set triggers` (which also > has a --from-webhook as well) ? > > > — > > You are receiving this because you authored the thread. > > Reply to this email directly, view it on GitHub > > <#13389 (comment) >, > > or mute the thread > > <https://github.com/notifications/unsubscribe-auth/ADbadCQp- > BGtNrnzGEBtpZMNXL7b7g97ks5rmAqJgaJpZM4MdLKN> > > > . > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#13389 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe- auth/ABG_pzjZf2BrAd2kjnK4AZk_itAmvUWrks5rmAwCgaJpZM4MdLKN> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADbadCmYJ9g5bG_-tG4CQobzVml-QxKoks5rmBE_gaJpZM4MdLKN> .

oatmealraisin · 2017-03-15T17:07:05Z

@coreydaley Thanks for the review! I've updated the PR based on your comments.

coreydaley · 2017-03-15T17:14:59Z

pkg/build/webhook/webhook_test.go

@@ -126,6 +162,40 @@ func TestFindTriggerPolicyMatchedGithubWebHook(t *testing.T) {
 	}
 }

+func TestFindTriggerPolicyMatchedGitlabWebHook(t *testing.T) {


coreydaley · 2017-03-15T17:15:11Z

pkg/build/generator/generator_test.go

+
+func TestInstantiateBuildTriggerCauseGitLabWebHook(t *testing.T) {
+	buildTriggerCauses := []buildapi.BuildTriggerCause{}
+	changeMessage := buildapi.BuildTriggerCauseGitlabMsg


coreydaley · 2017-03-15T17:15:21Z

pkg/build/registry/buildconfig/webhook_test.go

+		if cause.GitLabWebHook.Secret != hiddenSecret {
+			t.Errorf("Expected obfuscated secret to be: %s", hiddenSecret)
+		}
+		if cause.Message != api.BuildTriggerCauseGitlabMsg {


coreydaley · 2017-03-15T17:57:54Z

pkg/build/api/v1/types.go

+
+	// GitLabWebHook represents data for a GitLab webhook that fired a specific
+	// build.
+	GitLabWebHook *GitLabWebHookCause `json:"gitLabWebHook, omitempty" protobuf:"bytes,5,opt,name=gitLabWebHook"`


remove space after comma

coreydaley · 2017-03-15T17:58:07Z

pkg/build/api/v1/types.go

+
+	// BitbucketWebHook represents data for a Bitbucket webhook that fired a
+	// specific build.
+	BitbucketWebHook *BitbucketWebHookCause `json:"bitbucketWebhook, omitempty" protobuf:"bytes,6,opt,name=bitbucketWebHook"`


remove space after comma, upcase H on hook

gabemontero · 2017-03-15T18:17:06Z

@smarterclayton @csrwng - I was talking with @bparees a bit about the oc set triggers specifics and we circled back to the "should oc new-app automatically add gitlab and bitbucket triggers" like they do with github and generic question.

Per the details I put in the PR's description, I think we had left sprint planning with a decision of not adding them, but even if I am mis-remembering, in any event, both @bparees and I are still on the fence.

What is y'all's respective takes? Note - earlier @smarterclayton said he was fine with us not updating new-app ... but that does not necessarily imply not being OK with updating new-app

oatmealraisin · 2017-03-15T18:48:18Z

@coreydaley Added your requests!

coreydaley · 2017-03-15T19:07:15Z

pkg/build/api/v1/types.go

+
+	// BitbucketWebHook represents data for a Bitbucket webhook that fired a
+	// specific build.
+	BitbucketWebHook *BitbucketWebHookCause `json:"bitbucketWebhook,omitempty" protobuf:"bytes,6,opt,name=bitbucketWebHook"`


consistency: capitalize the h in Webhook

smarterclayton · 2017-03-15T19:30:18Z

I'm still ok not adding them automatically. They increase object size significantly. And they're just not used that often.

gabemontero · 2017-03-15T19:39:24Z

OK I've pushed the trigger changes for gitlab/bitbucket ... per @smarterclayton we are NOT automatically adding gitlab / bitbucket on new-app

gabemontero · 2017-03-22T22:21:50Z

Yeah, the wait for a build stuff is the mystery. Some tracing there would have helped, but perhaps tracing would be too chatty in the normal case ??

In fact, of you look at the build log dump after the build describe, it appears that the build completed successfully, including the push:

2017-03-22T19:16:01.775294000Z Successfully built 56fe6837c429
2017-03-22T19:16:01.784125000Z Pushing image 172.30.98.5:5000/extended-test-docker-build-pullsecret-z64pc-vv229/image1:latest ...
2017-03-22T19:16:02.504076000Z Pushed 0/1 layers, 3% complete
2017-03-22T19:16:35.356651000Z Pushed 1/1 layers, 100% complete
2017-03-22T19:16:36.106002000Z Push successful

Certainly seems more test framework than code base ... going to try and test again.

[test]

gabemontero · 2017-03-23T12:36:04Z

I'll push the necessary rebase updates shortly

gabemontero · 2017-03-23T14:23:30Z

OK post-rebase the test just passed again, and a second one is now running (perhaps the extra test comment I posted yesterday)

gabemontero · 2017-03-23T15:52:49Z

the second test hit 2 known flakes ... flake #13067 and flake #12072

other than that everything passed (and the prior test passed completely)

with the api approval (thanks @liggitt ) a question to @bparees - assuming we can debug the start build and wait issue in a separate pull (I can boot that up today) and that it seens ext test infrastructure related since the build actually succeeded, and the latest failures being known flakes, with a successful test run sandwiched in between, is this in "ygtm" state, and we should start the fork ami process?

bparees · 2017-03-23T16:01:12Z

a question to @bparees - assuming we can debug the start build and wait issue in a separate pull (I can boot that up today) and that it seens ext test infrastructure related since the build actually succeeded, and the latest failures being known flakes, with a successful test run sandwiched in between, is this in "ygtm" state, and we should start the fork ami process?

yup.

gabemontero · 2017-03-23T16:04:17Z

cool - thx @bparees

gabemontero · 2017-03-24T13:50:55Z

going to add some temporary debug in hack/build-base-images.sh via a separate commit to this branch to debug the issue creating fork ami's ... i'll revert the debug commit when things are sorted out.

I previously checked out this pr's branch in an aws instance I spun up yesterday and ran the vagrant build-base-images command successfully.

openshift-bot · 2017-03-27T20:45:20Z

Evaluated for origin test up to 9d300e8

openshift-bot · 2017-03-27T21:54:41Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/475/) (Base Commit: 959be87)

bparees · 2017-04-03T15:18:24Z

merging as is. We will revisit refactoring the existing webhookcause structs to use the new common embedded struct once @smarterclayton is back/weighs in on the protobuf implications.

[merge]

gabemontero · 2017-04-03T16:10:13Z

The merge jobs are still cooking, but we already have the conformance install setup hitting something that looks similar to flake #13594

bparees · 2017-04-03T16:50:40Z

flake #13606 though i'm not convinced it's a flake vs broken.

[merge]

gabemontero · 2017-04-03T18:17:18Z

same issue with https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/235/ as with https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/234/

Certainly seems more like a consistent problem vs. flake

gabemontero · 2017-04-03T18:18:58Z

Seems like a long shot, but just in case, any chance rebasing this PR to the latest origin would help things?

bparees · 2017-04-03T18:31:12Z

[merge]

…

On Mon, Apr 3, 2017 at 2:05 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/merge FAILURE ( https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/235/) (Base Commit: 889fcc4 <889fcc4> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3qnp0ubbpz57toSNrRH35dlIl-I7ks5rsTT0gaJpZM4MdLKN> .

-- Ben Parees | OpenShift

bparees · 2017-04-03T18:33:04Z

Seems like a long shot, but just in case, any chance rebasing this PR to the latest origin would help things?

rebasing will never affect the results of a test/merge, PRs are always tested/merged by applying the PR changes onto the latest master code (which is the same thing a rebase would do).

@stevekuznetsov thinks he's fixed the issue these PRs were hitting, hopefully the merge will go through now.

openshift-bot · 2017-04-03T18:37:21Z

Evaluated for origin merge up to 9d300e8

openshift-bot · 2017-04-03T20:05:33Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/237/) (Base Commit: 8b6ea7f) (Image: devenv-rhel7_6114)

coreydaley reviewed Mar 15, 2017

View reviewed changes

smarterclayton added the needs-api-review label Mar 15, 2017

oatmealraisin force-pushed the new-webhooks branch from c6d73d2 to 6c909d2 Compare March 15, 2017 17:04

coreydaley reviewed Mar 15, 2017

View reviewed changes

oatmealraisin force-pushed the new-webhooks branch from 6c909d2 to c244872 Compare March 15, 2017 18:45

coreydaley reviewed Mar 15, 2017

View reviewed changes

gabemontero force-pushed the new-webhooks branch from c244872 to a804f94 Compare March 15, 2017 19:38

gabemontero force-pushed the new-webhooks branch from a804f94 to bb1d6b9 Compare March 15, 2017 19:42

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

gabemontero force-pushed the new-webhooks branch from b4b8d16 to 7e0966f Compare March 23, 2017 13:00

oatmealraisin force-pushed the new-webhooks branch from 7e0966f to 7a8cbfd Compare March 23, 2017 13:55

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

gabemontero force-pushed the new-webhooks branch 2 times, most recently from 7ca43e3 to f71456f Compare March 24, 2017 13:15

support for gitlab and bitbucket webhooks

9d300e8

gabemontero force-pushed the new-webhooks branch from a3fdd34 to 9d300e8 Compare March 27, 2017 20:37

oatmealraisin mentioned this pull request Mar 30, 2017

Documented new GitLab and Bitbucket webhooks openshift/openshift-docs#4043

Merged

openshift-bot merged commit 3b82ca6 into openshift:master Apr 3, 2017

gabemontero mentioned this pull request Apr 4, 2017

complete investigation/change around use of common webhook cause for github/generic #13620

Closed

gabemontero deleted the new-webhooks branch April 5, 2017 14:15

support for gitlab and bitbucket webhooks #13389

support for gitlab and bitbucket webhooks #13389

Conversation

gabemontero commented Mar 14, 2017

coreydaley Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

coreydaley Mar 15, 2017 • edited Loading

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

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

gabemontero commented Mar 15, 2017

smarterclayton commented Mar 15, 2017

gabemontero commented Mar 15, 2017 via email

smarterclayton commented Mar 15, 2017 via email

gabemontero commented Mar 15, 2017 via email

oatmealraisin commented Mar 15, 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

coreydaley Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

gabemontero commented Mar 15, 2017 • edited Loading

oatmealraisin commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 15, 2017

gabemontero commented Mar 15, 2017

gabemontero commented Mar 22, 2017

gabemontero commented Mar 23, 2017

gabemontero commented Mar 23, 2017

gabemontero commented Mar 23, 2017

bparees commented Mar 23, 2017

gabemontero commented Mar 23, 2017

gabemontero commented Mar 24, 2017 • edited Loading

openshift-bot commented Mar 27, 2017

openshift-bot commented Mar 27, 2017

bparees commented Apr 3, 2017

gabemontero commented Apr 3, 2017 • edited Loading

bparees commented Apr 3, 2017

gabemontero commented Apr 3, 2017

gabemontero commented Apr 3, 2017

bparees commented Apr 3, 2017 via email

bparees commented Apr 3, 2017

openshift-bot commented Apr 3, 2017

openshift-bot commented Apr 3, 2017 • edited Loading

coreydaley Mar 15, 2017 •

edited

Loading

coreydaley Mar 15, 2017 •

edited

Loading

coreydaley Mar 15, 2017 •

edited

Loading

gabemontero commented Mar 15, 2017 •

edited

Loading

gabemontero commented Mar 24, 2017 •

edited

Loading

gabemontero commented Apr 3, 2017 •

edited

Loading

openshift-bot commented Apr 3, 2017 •

edited

Loading