Mm 379 - Add a feature to configure the webhook with the command itself

[maisnamrajusingh]

Summary

ability to configure the webhook using a command

Ticket Link

https://github.com/mattermost/mattermost-plugin-github/issues/379

[mattermod]

Hello @maisnamrajusingh,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

[maisnamrajusingh]

@hanzei Ignore the request, please.This still has some linting issues

[codecov-commenter]

Codecov Report

Merging #456 (9b87a40) into master (431bac0) will decrease coverage by 0.79%. The diff coverage is 6.02%.

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   18.49%   17.69%   -0.80%     
==========================================
  Files          12       12              
  Lines        3320     3549     +229     
==========================================
+ Hits          614      628      +14     
- Misses       2667     2882     +215     
  Partials       39       39              

Impacted Files	Coverage Δ
server/plugin/command.go	6.65% <0.00%> (-2.11%)	:arrow_down:
server/plugin/webhook.go	0.00% <0.00%> (ø)
server/plugin/plugin.go	4.08% <16.66%> (+0.19%)	:arrow_up:
server/plugin/template.go	95.45% <100.00%> (+0.23%)	:arrow_up:

---

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 431bac0...9b87a40. Read the comment docs.

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

[jasonblais]

@hanzei kind reminder to help with review

[mickmister]

@hanzei Is there a benefit to setting up webhooks via plugin other than for the subscriptions feature? If not, I propose we keep the webhooks an implementation detail and have the plugin handle the logistics of webhooks. See my comment here https://github.com/mattermost/mattermost-plugin-github/pull/466#discussion_r685694061

[sibasankarnayak]

@hanzei can you check on comments made and let me know

[hanzei]

@sibasankarnayak What comments are you referring to? I don't see an answer from you to the comment I've made in https://github.com/mattermost/mattermost-plugin-github/pull/456#pullrequestreview-803612426.

[sibasankarnayak]

@sibasankarnayak What comments are you referring to? I don't see an answer from you to the comment I've made in #456 (review).

@hanzei not sure why the commets were in pending state , can you check now

[sibasankarnayak]

@mickmister @hanzei

have added the org scope for oauth to manage in org level webhook ,let me know the review on this

[sibasankarnayak]

@hanzei @mickmister and i had a discussion on command /github webhook list org/[repo] , github won't allow us to add duplicate webhook so ultimately it is just one webhook every time and for getting that webhook we currently fetch all and filter out on basis on webhook call back url , so need you input on this should we keep the list command . michael had a suggestion for changing the command to /github webhook check org/repo.

[sibasankarnayak]

Two comments from testing the feature:

1. Can we improve the UX for users who didn't run the connect command again to approve the new scopes? Right now, they only get an error message.
2. I still can't get the feature working. The error message I get is

but the OAuth App is already approved:

Is there anything that I need to enable?

3. Running a per-repo command still returns a 404. Is that related to 2.?

@hanzei getting 404 error indicates the repo or org you are trying is not reachable , can you try to revoke all the user from the OAuth and try to disconnect and then connect . this should work.

[mickmister]

michael had a suggestion for changing the command to /github webhook check org/repo.

Sorry when I said "It seems like it should be /webhook check org/repo since we're only expecting at most one webhook right?" I meant that if we had this exact same command that it seems like it should be named that. Based on our conversation, I'm not necessarily suggesting that we have the command that takes the org/repo at all.

I like the idea of storing the webhooks created through the plugin and using that to list out the commands as suggested here https://github.com/mattermost/mattermost-plugin-github/pull/456#discussion_r761304353. We do need to take into account webhooks that have been deleted using the GitHub UI though. So we should check GitHub's API whenever the /github webhook list command is run, and remove webhook entries from the kv store that we find out have been deleted.

[sibasankarnayak]

michael had a suggestion for changing the command to /github webhook check org/repo.

Sorry when I said "It seems like it should be /webhook check org/repo since we're only expecting at most one webhook right?" I meant that if we had this exact same command that it seems like it should be named that. Based on our conversation, I'm not necessarily suggesting that we have the command that takes the org/repo at all.

I like the idea of storing the webhooks created through the plugin and using that to list out the commands as suggested here #456 (comment). We do need to take into account webhooks that have been deleted using the GitHub UI though. So we should check GitHub's API whenever the /github webhook list command is run, and remove webhook entries from the kv store that we find out have been deleted.

@mickmister just came accross this situation where user have already created the webhook manually and it points to mmSiteUrl , are we going to fetch the hook details and then storing it in KVStore ?

cc @hanzei

[sibasankarnayak]

@hanzei @mickmister have tried to store the webhook details in KVstore , please share feedback

[sibasankarnayak]

We talked offline about improving the UX if a user hasn't consented to new list of scopes yet. Was there any progress about that idea?

@hanzei i have got some , can have a look on this https://docs.github.com/en/developers/apps/building-oauth-apps/scopes-for-oauth-apps

[sibasankarnayak]

We talked offline about improving the UX if a user hasn't consented to new list of scopes yet. Was there any progress about that idea?

@hanzei i have got some , can have a look on this https://docs.github.com/en/developers/apps/building-oauth-apps/scopes-for-oauth-apps

this is the screenshot of message on insufficient scope , can you help me with the message for better ux

[hanzei]

@maisnamrajusingh Is it enough to /github connect without calling /github disconnect before?

[sibasankarnayak]

@maisnamrajusingh Is it enough to /github connect without calling /github disconnect before?

@hanzei yes we can directly use /github connect and it does replaces the scope

and one more thing i have a suggestion from my side

when the repo or org is invalid we get such error

instead of this we can have some meaningful error like invalid org/repo name as we have already able to figure if the error is because of scope

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @aspleenic

[hanzei]

This work conflicts with the work on the growth spike. Hence, work on it should stop for now.

[hanzei]

https://github.com/mattermost/mattermost-plugin-github/issues/379#issuecomment-1437213625