Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-86: Notification in channel on MR reopen #113

Merged
merged 3 commits into from
Dec 18, 2019
Merged

GH-86: Notification in channel on MR reopen #113

merged 3 commits into from
Dec 18, 2019

Conversation

ashishbhate
Copy link
Contributor

Send notification in channel on MR reopen. Fixes #86

I had to update my go.mod file to get the plugin to work. I'm not sure if the CI needs it.

See messages starting from https://community.mattermost.com/core/pl/nj3bdn8b8ifjfp9r8w41frk84w

@levb levb requested review from cpoile and jfrerich November 5, 2019 20:47
@ashishbhate
Copy link
Contributor Author

CI is failing with the same error I was seeing with the dependency versions in the go.mod file. There's a link in the OP to the community server chat log where I've posted how I fixed this error.

@levb levb added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 5, 2019
@levb
Copy link
Contributor

levb commented Nov 5, 2019

The build is failing because of go.mod and go 1.13, fix should be coming to master shortly.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ashishbhate -- just a suggestion to clean up the code a bit.

server/webhook/merge_request.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending change request from @cpoile! Thanks, @ashishbhate!

@ashishbhate
Copy link
Contributor Author

Updated with switch instead of long if-else. Let me know if you need me to squash the two commits into a single commit.

@levb
Copy link
Contributor

levb commented Nov 9, 2019

@ashishbhate can you please sync and merge with master to fix the build?

@levb levb removed 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Nov 9, 2019
@ashishbhate
Copy link
Contributor Author

@levb I've merged master into the branch

@hanzei hanzei added this to the v1.1.0 milestone Nov 10, 2019
@mattermod
Copy link
Contributor

This issue 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 @hanzei

@jasonblais
Copy link
Contributor

@DHaussermann Friendly reminder to help with QA review :)

@DHaussermann
Copy link

@levb this PR is still untested as I unable to build this. Let's follow-up so I can get assistance from someone on the team to see what's going on.

@ashishbhate
Copy link
Contributor Author

Hi guys, let me know if there's anything I can do to help get this PR merged.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed
Created a webhook and confirmed that re-opening a merge request delivers an event as expected
LGTM!
Thanks @ashishbhate for fixing this issue. Apologies for the delay on this one.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 18, 2019
@hanzei hanzei merged commit 9622cdb into mattermost:master Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-opening a MR doesn't cause post a message
9 participants