[GH-15] manage webhooks with slash commands #137

ericjaystevens · 2020-02-28T02:59:35Z

Summary

Add features to manage projects and group scoped GitLab webhooks.

/gitlab webhook list group[/project]
/gitlab webhook add group[/project] [url] [triggers] [token]-- if url is omitted points to plugin URL, if triggers is blank sets all, if token is omitted uses plugin's token

When a user subscribes to a new project or group, it will tell them the command to run to create a webhook, if one isn't already created for the group/project.

TODO

list project hooks
list group hooks
create project hooks
create group hooks

Ticket Link

fixes #15
fixes #156

codecov · 2020-03-10T01:31:53Z

Codecov Report

Merging #137 into master will increase coverage by 2.51%.
The diff coverage is 70.88%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   39.79%   42.31%   +2.51%     
==========================================
  Files          16       16              
  Lines        1495     1633     +138     
==========================================
+ Hits          595      691      +96     
- Misses        840      871      +31     
- Partials       60       71      +11

Impacted Files	Coverage Δ
server/api.go	`0.00% <0.00%> (ø)`
server/plugin.go	`14.97% <56.25%> (+3.45%)`	⬆️
server/command.go	`40.20% <74.10%> (+23.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c1eab3...3314c7b. Read the comment docs.

ericjaystevens · 2020-04-03T15:14:07Z

I've made all the required upstream changes, and it was just released today https://github.com/xanzy/go-gitlab/releases/tag/v0.30.0

a little more work on my end and this should be ready to move forward.

Added user message based on Web hook status when /gitlab subscribe is used. (#130) Message alerting the user to the need to add a webhook is functioning. Additional refactoring and tests needed. single webhook test with mocking refactored project hook tests added no webhooks test moved mock function to gitlabtest package replaced manual mocks with auto generated refactored plugin command handling and clarified messages added api error test removed mocks that are no longer needed removed test for GetProjectHooks as it seems to implementation-specific go format removed shadow declaration based on vet output broke out subscribeComand method to improve testability tested no subscripions paramaterized testSubscribeCommand tests mocked KVSet tests webhook found moved tests out of plugin_test and into command_test fixed linting issues fixed Error identifier Co-Authored-By: Michael Kochell <mjkochell@gmail.com> fixed Error identifier Co-Authored-By: Michael Kochell <mjkochell@gmail.com> spelling Co-Authored-By: Michael Kochell <mjkochell@gmail.com> generated mock with gomock works for all test except no Substitutions all tests pass changed mock boolean flag name WIP

new webhook test hard coded wip added trigger default logic creates webhook with nice output formated project hooks properly explicit trigers works with explicit URL wip improved hook formating

compiles

tested add group hook abstracted group and project hook options updated subscribe messages fixed spelling rebased with upsream master

ericjaystevens · 2020-04-10T15:30:25Z

feature complete, @hanzei Work in Progress can be removed.

go mod tidy merged go.sum conlict go tidy

hanzei · 2020-04-10T19:09:04Z

@ericjaystevens Heads up that there is a merge conflict to resolve

iomodo

Looks good to me! Thanks for the contribution @ericjaystevens, great work!
One question below.

iomodo · 2020-04-13T13:01:07Z

server/command.go

+		fullPath := strings.Split(namespace, "/")
+
+		siteURL := *p.API.GetConfig().ServiceSettings.SiteURL
+		urlPath := fmt.Sprintf("%v/%s", siteURL, inboundWebhookURL)


Shouldn't we add the port as well in case it is not :80?

Good Thinking, I use port 8065 in my test environment. But I have the port number included in my site URL.

Looking at the System Console under site URL I see,

The URL that users will use to access Mattermost. Standard ports, such as 80 and 443, can be omitted, but non-standard ports are required. For example: http://example.com:8065. This setting is required.

So I think it might be safe to assume the port will be included in the Site URL. But if that's not a safe assumption, it should be an easy check.

You are absolutely right. One case worth checking might be siteURL being empty, because it's a default. I'd suggest using

"localhost" + *p.API.GetConfig().ServiceSettings.ListenAddress

I think it's a good idea to check siteURL for empty, I'll add that.

I'm pretty sure other things won't work (like subscribe and activate) if siteURL is not set. But if gets unset webhook creation would set webhooks to point to just the inboundWebhookURL, which would be unexpected.

I'm not sure computing the URL is practical or desired, they could be behind a load balancer or proxy and we wouldn't know their external IP/FQDN.

How about I just add a check, and if it is empty I fail the webhook creation with a message like

Unable to create webhook. The Mattermot Site URL is not set. Set it in the admin console or rerun /gitlab webhook add group/project URL including the desired URL.

iomodo

Thanks @ericjaystevens, looks good to me!

hanzei · 2020-05-07T09:23:15Z

/update-branch

hanzei

Awesome work 🎉

I leaved a few suggestions

hanzei · 2020-05-07T09:25:01Z

server/command.go

+	 * PipelineEvents 
+	 * WikiPageEvents
+	 * SSLverification
+  * |url| is the URL that will be called when triggered. Defaults to this plugins URL


What is a use case for setting this not to the plugin URL?

creating a webhook for your CI/CD tooling or another chat or notification service

Can you elaborate a bit? Is the exact use case that a user wan't to create a webhook to an different service inside of Mattermost?

Yes, An example user story would be a developer creates a new GitLab repo, and wants to get it added to the team's Jenkins server. So the developer messages the Jenkins administrator to get the token and Jenkins URL for the webhook. Then the developer runs the Mattermost Gitlab Plugin command and creates the webhook to Jenkins.

server/command.go

server/gitlab/api.go

server/gitlab/gitlab.go

server/plugin.go

hanzei

Great work 🎉

server/command.go

use ! instead of == false Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>

hanzei · 2020-06-03T10:35:09Z

@iomodo Do you have a local GitLab instance setup to test these changes?

iomodo · 2020-06-03T10:58:16Z

@hanzei it's on my todo list.

iomodo

Started testing this PR on a local instance. Found a bug I missed in the review.

iomodo · 2020-06-07T15:37:52Z

server/gitlab/gitlab.go

-	client := internGitlab.NewOAuthClient(tc, token.AccessToken)
-	if err := client.SetBaseURL(g.enterpriseBaseURL); err != nil {
-		return nil, fmt.Errorf("can't set base url to GitLab client lib: %w", err)
+	client, err := internGitlab.NewOAuthClient(token.AccessToken)


Not setting base url will make requests go to the default gitlab.com URL.
What was the reason for changing gitlabConnect method?

You are absolutely right. The reason for the modifications to gitlabConnect was because of changes in go-gitlab. The types of arguments for NewOAuthClient changed, and SetBaseURL became a private function. I must have changed it to get this feature to compile and the forgot that the gitlabConnect method needs a re-write.

I just pushed a change and tested it with Gitlab.com. Here is the link to the breaking change commit that I updated gitlabConnect to address. xanzy/go-gitlab#826

…ns/mattermost-plugin-gitlab into feature_manageHooks

iomodo

Tested locally, works as expected. Could not test groups though.
Great job @ericjaystevens !

hanzei · 2020-06-11T09:05:59Z

@DHaussermann ☝️

DHaussermann

Thanks for testing @iomodo. I have added this to release testing so it is included in end to end testing in the next release.
LGTM!
Huge thanks @ericjaystevens for this contribution.

bbodenmiller · 2021-02-18T10:18:37Z

Looks like https://mattermost.gitbook.io/plugin-gitlab/setup/configuration needs to be updated following this change.

hanzei · 2021-02-19T10:56:37Z

@ericjaystevens Would you be open on updating the documentation?

hanzei · 2021-02-19T10:56:46Z

@bbodenmiller Thanks for the hint 👍

hanzei requested a review from manland February 28, 2020 12:05

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 28, 2020

hanzei added Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 24, 2020

hanzei removed the request for review from manland March 24, 2020 11:17

hanzei and others added 4 commits April 10, 2020 15:26

Added webhook list group

690dc4b

new webhook test hard coded wip added trigger default logic creates webhook with nice output formated project hooks properly explicit trigers works with explicit URL wip improved hook formating

adds support for creating project hooks

f0cab28

compiles

add Group Hooks

2b133ba

tested add group hook abstracted group and project hook options updated subscribe messages fixed spelling rebased with upsream master

feature manage web hooks resolves #15

0933275

go mod tidy merged go.sum conlict go tidy

hanzei requested a review from iomodo April 10, 2020 19:08

hanzei added 2: Dev Review Requires review by a core committer and removed Work In Progress Not yet ready for review labels Apr 10, 2020

fixed go.sum merge conflict

0c2da85

ericjaystevens requested a review from hanzei as a code owner April 10, 2020 19:38

hanzei changed the title ~~WIP: [GH-15] manage webhooks with slash commands~~ [GH-15] manage webhooks with slash commands Apr 13, 2020

iomodo reviewed Apr 13, 2020

View reviewed changes

improved error handling for empty Site URL

33f613f

iomodo approved these changes Apr 14, 2020

View reviewed changes

manland mentioned this pull request Apr 15, 2020

clarify how average user can use plugin without access to webhook secret #156

Closed

gabrieljackson requested review from hanzei and removed request for hanzei April 20, 2020 15:18

merge conflict fixes spelling of assigned

bb3c97b

Merge branch 'master' into feature_manageHooks

4fcdbff

hanzei requested changes May 7, 2020

View reviewed changes

changes based on hanzei's code review

5b0f7a6

hanzei approved these changes May 9, 2020

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

hanzei added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels May 9, 2020

hanzei requested a review from DHaussermann May 9, 2020 00:27

hanzei added this to the v1.2.0 milestone May 9, 2020

hanzei mentioned this pull request May 9, 2020

Release v1.2.0 #170

Closed

Update server/command.go

26cb9df

use ! instead of == false Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>

iomodo suggested changes Jun 7, 2020

View reviewed changes

hanzei and others added 3 commits June 8, 2020 13:33

Merge branch 'master' into feature_manageHooks

c1938f7

updated gitlabConnect to use updated API calls

48ce410

Merge branch 'feature_manageHooks' of https://github.com/ericjaysteve…

3314c7b

…ns/mattermost-plugin-gitlab into feature_manageHooks

ericjaystevens requested a review from iomodo June 9, 2020 01:39

iomodo approved these changes Jun 10, 2020

View reviewed changes

DHaussermann approved these changes Jun 11, 2020

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jun 11, 2020

hanzei merged commit e5d1e89 into mattermost:master Jun 11, 2020

iomodo mentioned this pull request Jul 29, 2020

Bump version to 1.2.0 #191

Merged

hanzei added the Docs/Needed Requires documentation label Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-15] manage webhooks with slash commands #137

[GH-15] manage webhooks with slash commands #137

ericjaystevens commented Feb 28, 2020 •

edited

Loading

codecov bot commented Mar 10, 2020 •

edited

Loading

ericjaystevens commented Apr 3, 2020

ericjaystevens commented Apr 10, 2020

hanzei commented Apr 10, 2020

iomodo left a comment

iomodo Apr 13, 2020

ericjaystevens Apr 13, 2020

iomodo Apr 13, 2020

ericjaystevens Apr 13, 2020

iomodo left a comment

hanzei commented May 7, 2020

hanzei left a comment

hanzei May 7, 2020

ericjaystevens May 7, 2020

hanzei May 9, 2020

ericjaystevens May 9, 2020

hanzei left a comment

hanzei commented Jun 3, 2020

iomodo commented Jun 3, 2020

iomodo left a comment

iomodo Jun 7, 2020

ericjaystevens Jun 8, 2020

ericjaystevens Jun 9, 2020

iomodo left a comment

hanzei commented Jun 11, 2020

DHaussermann left a comment

bbodenmiller commented Feb 18, 2021

hanzei commented Feb 19, 2021

hanzei commented Feb 19, 2021

[GH-15] manage webhooks with slash commands #137

[GH-15] manage webhooks with slash commands #137

Conversation

ericjaystevens commented Feb 28, 2020 • edited Loading

Summary

TODO

Ticket Link

codecov bot commented Mar 10, 2020 • edited Loading

Codecov Report

ericjaystevens commented Apr 3, 2020

ericjaystevens commented Apr 10, 2020

hanzei commented Apr 10, 2020

iomodo 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

iomodo left a comment

Choose a reason for hiding this comment

hanzei commented May 7, 2020

hanzei 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

hanzei left a comment

Choose a reason for hiding this comment

hanzei commented Jun 3, 2020

iomodo commented Jun 3, 2020

iomodo 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

iomodo left a comment

Choose a reason for hiding this comment

hanzei commented Jun 11, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

bbodenmiller commented Feb 18, 2021

hanzei commented Feb 19, 2021

hanzei commented Feb 19, 2021

ericjaystevens commented Feb 28, 2020 •

edited

Loading

codecov bot commented Mar 10, 2020 •

edited

Loading