Support for Gitlab and Bitbucket webhooks in the BC editor #1539

jhadvig · 2017-05-11T12:50:01Z

Adding support for view/add/remove GitLab and Bitbucket webhooks.
Screen:

Not really sure if we want to display them by default as the screenshot show, but also not sure if we want to hide them in some "Advanced webhooks settings" since we already have advance view in the BC editor.

@spadgett @jwforres thoughts ?

jwforres · 2017-05-11T14:16:08Z

what if the webhook types were in a dropdown where you could select one and hit add, and then if there are in a particular type you show them like you have there. there might be a better way to layout the webhooks as well instead of in a separated list like that, might be able to have a table where the type is one of the columns. definitely think there are other ways we could represent this

…

On Thu, May 11, 2017 at 8:50 AM, Jakub Hadvig ***@***.***> wrote: Adding support for view/add/remove GitLab and Bitbucket webhooks. Screen: [image: screenshot at 2017-05-11 14-09-39] <https://cloud.githubusercontent.com/assets/1668218/25949240/1bfc7736-3657-11e7-8e2d-a4440d8acdc4.png> [image: screenshot at 2017-05-11 14-38-43] <https://cloud.githubusercontent.com/assets/1668218/25949380/a3529f94-3657-11e7-976c-e6136bd25646.png> Not really sure if we want to display them by default as the screenshot show, but also not sure if we want to hide them in some "Advanced webhooks settings" since we already have advance view in the BC editor. @spadgett <https://github.com/spadgett> @jwforres <https://github.com/jwforres> thoughts ? ------------------------------ You can view, comment on, or merge this pull request online at: #1539 Commit Summary - Support for Gitlab and Bitbucket webhooks in the BC editor File Changes - *M* app/scripts/controllers/edit/buildConfig.js <https://github.com/openshift/origin-web-console/pull/1539/files#diff-0> (10) - *M* app/scripts/directives/editWebhookTriggers.js <https://github.com/openshift/origin-web-console/pull/1539/files#diff-1> (16) - *M* app/views/directives/edit-webhook-triggers.html <https://github.com/openshift/origin-web-console/pull/1539/files#diff-2> (2) - *M* app/views/edit/build-config.html <https://github.com/openshift/origin-web-console/pull/1539/files#diff-3> (16) - *M* dist/scripts/scripts.js <https://github.com/openshift/origin-web-console/pull/1539/files#diff-4> (36) - *M* dist/scripts/templates.js <https://github.com/openshift/origin-web-console/pull/1539/files#diff-5> (6) Patch Links: - https://github.com/openshift/origin-web-console/pull/1539.patch - https://github.com/openshift/origin-web-console/pull/1539.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1539>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7WOR6--YrtqvBtlmxfRLZfrf9GB9ks5r4wP_gaJpZM4NX8vn> .

jwforres · 2017-05-11T14:16:28Z

@ncameronbritt

ncameronbritt · 2017-05-11T14:20:50Z

@jwforres Sound like you're suggesting something similar to the way we handle users/roles in Membership?

ncameronbritt · 2017-05-12T12:29:50Z

What about something like this:

ncameronbritt · 2017-05-12T12:33:55Z

Is having multiple (and multiple kinds of) webhooks something that users need?

jwforres · 2017-05-12T12:54:25Z

The most likely scenario is having one of the source webhooks (github/gitlab/bitbucket) and then having the generic webhook. You might trigger the generic webhook from something in your CI infrastructure. But you might also have a Jenkins pipeline that pulls source from multiple repos as part of a larger build workflow, in that case you might want any one of those repos to trigger webhooks, odds are in this case that you'll be using the same repo type (e.g. GitHub) and could share the same webhook. But its definitely possible for you to have repos in different systems, or you might just not want to share the same webhook secret across different repos.

…

On Fri, May 12, 2017 at 8:33 AM, Cameron Britt ***@***.***> wrote: Is having multiple (and multiple kinds of) webhooks something that users need? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7dnlJxwSwvmL1xkzvcm6yiQubqX0ks5r5FG3gaJpZM4NX8vn> .

jhadvig · 2017-05-12T13:01:01Z

@jwforres @ncameronbritt so I was thinking that we could list only existing webhooks, but instead of having a link under each type for Add {{type}} Webhook we could have a selectbox to specify the webhook type with a Add btn at the end of the wwebhook list.

jwforres · 2017-05-12T13:10:39Z

yeah something like that could work. FYI cameron the user doesn't have to enter anything in order to add the webhook, the URL input is just for them to copy once the URL is generated for them.

…

On Fri, May 12, 2017 at 9:01 AM, Jakub Hadvig ***@***.***> wrote: @jwforres <https://github.com/jwforres> @ncameronbritt <https://github.com/ncameronbritt> so I was thinking that we could list only existing webhooks, but instead of having a link under each type for Add {{type}} Webhook we could have a selectbox to specify the webhook type with a Add btn at the end of the wwebhook list. [image: openshift web console] <https://cloud.githubusercontent.com/assets/1668218/25998992/c1af03ac-3723-11e7-8703-4a7d114e2680.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7dQM8CBMuHkHMOAxbylOyKXXneoqks5r5FgSgaJpZM4NX8vn> .

ncameronbritt · 2017-05-12T13:55:36Z

Here are a few sketches. The first example seems like what @jhadvig proposed. Seems slightly odd that we would have to add a header for a new type. And how would they order? What if you had more than one for a given type?

The others are riffs on using more of a table format. The main difference being how the "add" action would work. Here I'd imagine that adding one would just append another row in the table.

jhadvig · 2017-05-12T14:24:18Z

@ncameronbritt so I was thinking something between #1 and #4. Dont think its necessary to have an extra Add webhook header.

OR

so we align with the style which we already have.
@jwforres thoughts ?

jhadvig · 2017-05-12T14:25:10Z

personally I would go with the the second option from my last comment

sspeiche · 2017-05-12T14:27:05Z

I can't see the different between the two but it looks good to me.

jhadvig · 2017-05-12T14:32:12Z

@sspeiche its the Add button... one aligns with the Remove/Undostyle we have in the existing webhooks, and the other is different.

ncameronbritt · 2017-05-12T18:09:22Z

I think I like the second one better, @jhadvig.

I still wonder about having them in more of a list with the label to the left of the URL and appending the most recently added to the end of the list. I think that might make it easier to keep up with which one is which if, for example, you had two GitHub webhooks. But that might not be a realistic scenario.

jhadvig · 2017-05-15T09:22:09Z

@ncameronbritt well then we would have to think about the way how to display the Remove/Undo logic in the list. I would suggest do that as a followup if we need to and just add the addtional types and the selectbox with the Add link as was suggested. Thoughts ? :)

ncameronbritt · 2017-05-15T13:13:29Z

I'm ok with that.

jhadvig · 2017-05-16T09:33:37Z

PR updated. Added logic for adding webhooks into editWebhookTriggers.js. When used the add-webhook attribute has to be specified ot the edit-webhook-triggers directive.
Attaching screens:

@jwforres @ncameronbritt PTAL

spadgett

Thanks @jhadvig. A few comments.

spadgett · 2017-06-06T18:44:54Z

app/scripts/directives/editWebhookTriggers.js

+              return null;
+          }
+        }
+        $scope.triggerMeta = { 


I think I'd do something like

$scope.createTriggerSelect = { selectedType: "", options: [{ type: 'github', label: 'GitHub' }, ... ] };

Then you don't need getWebhookType above. You can update ui-select-choices to make the label option.label

spadgett · 2017-06-06T18:54:18Z

app/scripts/directives/editWebhookTriggers.js

            secret: ApplicationGenerator._generateSecret()
          };
-          $scope.triggers.push(webhook);
-          $scope.form.$setDirty();
+          $scope.triggers[webhookType + "Webhooks"].push(webhook);


i feel think the edit-webhook-triggers should take in just the array for this type instead of the entire map. Then you push push onto the array and don't need to worry about adding + "Webhooks" to the name.

But we have to pass the entire map cause we dont know what kind of webhook will user choose. When he chooses GitHub and creates it, we have to add the new webhook to the githubWebhooks array, or bitbucketWebhooks if he chooses Bitbucket. For that case we need to pass the entire map, just for the creating webhook action. Other option would be to split the create and view action for the webhooks in the edit-webhook-triggers and have separate directives for them. Thoughts ?

spadgett · 2017-06-06T18:55:12Z

app/styles/_core.less

+
+  .col-add-webhook {
+    padding-top: 5px;
+    padding-bottom: 10px;


Nit: We've started to alphabetize styles in our less files

spadgett · 2017-06-06T18:57:04Z

app/views/directives/edit-webhook-triggers.html

+<div class="display-webhooks" ng-if="!addWebhook">
+  <h5>{{type}} webhooks
+    <span class="help action-inline">
+      <a href="" aria-hidden="true">


If this is aria-hidden then the sr-only span on the next line will be hidden from screen readers. I'd move it outside the a element.

spadgett · 2017-06-06T18:58:39Z

app/views/directives/edit-webhook-triggers.html

-</h5>
-<div ng-repeat="trigger in triggers">
+<div class="display-webhooks" ng-if="!addWebhook">
+  <h5>{{type}} webhooks


{{type}} Webhooks

(capital W)

spadgett · 2017-06-06T19:01:13Z

app/views/directives/edit-webhook-triggers.html

+  </div>
+</div>
+<div class="add-webhook" ng-if="addWebhook">
+  <h5>Add webhook</h5>


Add Webhook

spadgett · 2017-06-06T19:01:32Z

app/views/directives/edit-webhook-triggers.html

  <div class="trigger-info">
    <span class="trigger-url">
-      <copy-to-clipboard is-disabled="trigger.disabled" clipboard-text="bcName | webhookURL : trigger.data.type : trigger.data[(type === 'GitHub') ? 'github' : 'generic'].secret : projectName"></copy-to-clipboard>
+      <ui-select ng-model="triggerMeta.typeToCreate"
+        theme="bootstrap"


I don't think theme="bootstrap" is needed

spadgett · 2017-06-06T19:03:38Z

app/views/edit/build-config.html

+                                ng-if="triggers.gitlabWebhooks.length"
+                                add-webhook="false"
+                                type="GitLab"
+                                type-info="A GitLab source repository must be configured to use the webhook to trigger a build when source is committed."


"The GitLab source repository..."

spadgett · 2017-06-06T19:03:50Z

app/views/edit/build-config.html

+                                ng-if="triggers.bitbucketWebhooks.length"
+                                add-webhook="false"
+                                type="Bitbucket"
+                                type-info="A Bitbucket source repository must be configured to use the webhook to trigger a build when source is committed."


"The Butbucket source repository...."

spadgett · 2017-06-06T19:18:35Z

app/views/directives/edit-webhook-triggers.html

-        <span class="fa fa-repeat" aria-hidden="true" title="Undo"></span>
-        <span class="sr-only">Undo</span>
+      <a href="" class="action-icon" ng-click="addWebhookTrigger(triggerMeta.typeToCreate)" role="button">
+        <span class="fa fa-plus" aria-hidden="true" title="Add"></span>


You should give the add buttons here and for mobile disabled styles when a type is not selected.

Not really sure what you mean... dont we want to have the same styling here as we have in the for displaying the WH ? Meaning for non-mobile we would have Add link and for mobile an icon, as displayed in #1539 (comment)

When I haven't selected anything in dropdown, I can't click the add button yet. It should have disabled classes so it's grayed out, cursor: not-allowed, etc.

Ah, I see. It's strange to me that edit-webhooks only edits webhooks of one type, but there's an add control for ALL types inside the directive. edit-webhooks directive should either be one directive for all webhooks or add should be outside the directive. Given the way the code is structured, I think moving add outside the directive is easier.

jhadvig · 2017-06-09T10:12:49Z

Comments addressed. @spadgett PTAL :)

spadgett

Thanks @jhadvig this looks a lot cleaner. Just a few small comments.

spadgett · 2017-06-12T21:26:42Z

app/scripts/controllers/edit/buildConfig.js

+      var webhook = {
+        disabled: false,
+        data: {
+          type: webhookLabel


Ah, I see that this is the API type value. Sorry didn't realize. I suggest:

options: [{ type: 'GitHub', propterty: 'github' }, { ... }]

And then this function should take in parameter type.

spadgett · 2017-06-12T21:28:42Z

app/scripts/controllers/edit/buildConfig.js

+          type: webhookLabel
+        }
+      };
+      var webhookType = _.find($scope.createTriggerSelect.options, function(o) {return o.label === webhookLabel}).type


Missing ;. You can use the shorthand here

var webhookType = _.find($scope.createTriggerSelect, { label: webhookLabel }).type; ...

or if you rename the properties as above, this would be

var property = _.find($scope.createTriggerSelect, { type: type }).property;

spadgett · 2017-06-12T21:31:17Z

app/scripts/controllers/edit/buildConfig.js

+      webhook.data[webhookType] = {
+        secret: ApplicationGenerator._generateSecret()
+      };
+      $scope.triggers[webhookType + "Webhooks"].push(webhook);


I wouldn't append "Webhooks" if we don't need to? Maybe just...?

$scope.triggers[webhookType].push(webhook)

Realize this affects the initialization code and the view some.

spadgett

Since my remaining comments are minor, I'm going to approve to make sure it's in for dev cut.

spadgett · 2017-06-13T19:35:28Z

[merge]

spadgett · 2017-06-13T20:24:45Z

Looks like test flake #1685

[merge]

openshift-bot · 2017-06-13T20:31:19Z

Evaluated for origin web console merge up to 0ce355d

openshift-bot · 2017-06-13T20:31:29Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1519/) (Base Commit: 54262a2)

jhadvig force-pushed the additionl_hooks branch from e91ce58 to a1cdc79 Compare May 16, 2017 09:34

spadgett self-requested a review June 6, 2017 14:29

spadgett self-assigned this Jun 6, 2017

spadgett requested changes Jun 6, 2017

View reviewed changes

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

jhadvig force-pushed the additionl_hooks branch from a1cdc79 to 0bbf3d6 Compare June 9, 2017 10:11

Support for Gitlab and Bitbucket webhooks in the BC editor

0ce355d

jhadvig force-pushed the additionl_hooks branch from 0bbf3d6 to 0ce355d Compare June 9, 2017 10:15

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

spadgett reviewed Jun 12, 2017

View reviewed changes

spadgett approved these changes Jun 13, 2017

View reviewed changes

openshift-bot merged commit 50bfbcc into openshift:master Jun 13, 2017

Support for Gitlab and Bitbucket webhooks in the BC editor #1539

Support for Gitlab and Bitbucket webhooks in the BC editor #1539

Conversation

jhadvig commented May 11, 2017 • edited by spadgett Loading

jwforres commented May 11, 2017 via email

jwforres commented May 11, 2017

ncameronbritt commented May 11, 2017

ncameronbritt commented May 12, 2017

ncameronbritt commented May 12, 2017

jwforres commented May 12, 2017 via email

jhadvig commented May 12, 2017

jwforres commented May 12, 2017 via email

ncameronbritt commented May 12, 2017 • edited Loading

jhadvig commented May 12, 2017

jhadvig commented May 12, 2017

sspeiche commented May 12, 2017

jhadvig commented May 12, 2017 • edited Loading

ncameronbritt commented May 12, 2017

jhadvig commented May 15, 2017

ncameronbritt commented May 15, 2017

jhadvig commented May 16, 2017

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig Jun 8, 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

jhadvig commented Jun 9, 2017

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Jun 13, 2017

spadgett commented Jun 13, 2017

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 13, 2017 • edited Loading

jhadvig commented May 11, 2017 •

edited by spadgett

Loading

ncameronbritt commented May 12, 2017 •

edited

Loading

jhadvig commented May 12, 2017 •

edited

Loading

jhadvig Jun 8, 2017 •

edited

Loading

spadgett Jun 12, 2017 •

edited

Loading

openshift-bot commented Jun 13, 2017 •

edited

Loading