Support for multiple channels #75

limod · 2023-03-28T14:52:26Z

Hi!

i added the feature to specify multiple channels. We have multiple environments and want to seperate your monitoring into different mattermost channels.

You can configure the field Channels to send notifications to include multiple channels separated by ; e.g. teamname,channelname;teamname-2,channelname-2

You can configure the target channel in the webhook URL, e.g. https://your-mattermost-url/plugins/com.mattermost.aws-sns?token=your-mattermost-token?channel=teamname,channelname

If you specify no channel query parameter, the first channel will be used. Therefore the change should be compatible with older version.

Added some tests: go test ./... -cover

Added the flag CGO_ENABLED=0 in the Makefile because i hat problems running the plugin in a dev mattermost setup (Image: mattermost/mattermost-team-edition:7.4.0): Plugin could not start becasue of error GLIBC_2.34 not found, with local development in the current mattermost-server project, everything worked without the flag. Any ideas why i hit this error?

mattermost-build · 2023-03-28T14:52:37Z

Hello @limod,

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.

codecov · 2023-03-28T20:59:47Z

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (363b452) 0.00% compared to head (d22dc40) 17.03%.
Report is 2 commits behind head on master.

❗ Current head d22dc40 differs from pull request most recent head d57bf24. Consider uploading reports for the commit d57bf24 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master      #75       +/-   ##
===========================================
+ Coverage        0   17.03%   +17.03%     
===========================================
  Files           0        4        +4     
  Lines           0      587      +587     
===========================================
+ Hits            0      100      +100     
- Misses          0      469      +469     
- Partials        0       18       +18

Files	Coverage Δ
server/command.go	`23.07% <0.00%> (ø)`
server/plugin.go	`15.22% <41.66%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister

Thanks for this great improvement @limod!

I have a few requests to avoid some possibly excessive logging, as well as a few other comments.

In general though LGTM 👍 Great work

plugin.json

server/plugin_test.go

README.md

server/plugin.go

mickmister · 2023-03-28T21:26:24Z

server/plugin.go

@@ -48,19 +61,18 @@ func (p *Plugin) OnActivate() error {
 		return err
 	}

-	split := strings.Split(p.configuration.TeamChannel, ",")
-	if len(split) != 2 {
+	teamChannels, err := parseTeamChannelsNames(p.configuration.TeamChannel)


Unrelated to the changes in this PR:

It's concerning that this is all happening in OnActivate, and not also triggered by OnConfigurationChange, since I believe this means that you have to restart the plugin for any config changes to take effect.

Created a ticket for this #77

limod · 2023-03-29T11:10:09Z

Accidentally removed @stylianosrigas, i am not able to readd him. @mickmister

stylianosrigas

LGTM, thanks @limod 🚀

mickmister

LGTM 👍 Thanks @limod!

mattermost-build · 2023-04-09T01:01:28Z

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!

limod · 2023-05-30T05:39:57Z

When will the review continue?

mickmister · 2023-07-06T21:47:54Z

@mkdbns @DHaussermann @jasonblais Thoughts on handling QA review on community contributions like this?

jasonblais · 2023-07-07T11:40:28Z

@mkdbns would this be an example contribution for your team to QA review & merge once your team has the proper GitHub access?

README.md

Kshitij-Katiyar · 2023-09-27T13:15:07Z

@limod @mickmister From our QA

Tested and Passed
The comment above is necessary for this PR to work and I have tested everything after making that change because without that the token will always be incorrect.

Tested the plugin by updating the system console values to include multiple channels and notifications are coming successfully.
Cases handled:-
i) Invalid channelID in both the system console and in the URL.
ii) URL without channelid also works fine.

Co-authored-by: kshitij katiyar <[email protected]>

mickmister · 2023-09-27T16:52:15Z

Thanks for the contribution @limod!

Support for multiple channels

d22dc40

limod requested a review from mickmister as a code owner March 28, 2023 14:52

mickmister requested changes Mar 28, 2023

View reviewed changes

mickmister requested a review from stylianosrigas March 28, 2023 21:30

mickmister mentioned this pull request Mar 28, 2023

Config updates should be processed in OnConfigurationChange #77

Open

MR feedback

fe112ed

limod mentioned this pull request Mar 29, 2023

Support posting AWS SNS alerts to more than one channel #14

Open

limod requested review from mickmister and removed request for stylianosrigas March 29, 2023 11:01

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 29, 2023

stylianosrigas approved these changes Mar 29, 2023

View reviewed changes

mickmister approved these changes Mar 29, 2023

View reviewed changes

mickmister requested a review from DHaussermann March 29, 2023 16:31

mickmister removed the 2: Dev Review Requires review by a core committer label Mar 29, 2023

mattermost-build added the Lifecycle/1:stale label Apr 9, 2023

Kshitij-Katiyar reviewed Sep 27, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

fix query param in readme instructions

d57bf24

Co-authored-by: kshitij katiyar <[email protected]>

mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Sep 27, 2023

mickmister approved these changes Sep 27, 2023

View reviewed changes

mickmister merged commit fbf9fef into mattermost-community:master Sep 27, 2023

avas27JTG added this to the v1.3.0 milestone Oct 30, 2023

avas27JTG mentioned this pull request Nov 30, 2023

Bumped version to 1.3.0. #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple channels #75

Support for multiple channels #75

limod commented Mar 28, 2023

mattermost-build commented Mar 28, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading

mickmister left a comment

mickmister Mar 28, 2023

mickmister Mar 28, 2023

limod commented Mar 29, 2023

stylianosrigas left a comment

mickmister left a comment

mattermost-build commented Apr 9, 2023

limod commented May 30, 2023

mickmister commented Jul 6, 2023

jasonblais commented Jul 7, 2023

Kshitij-Katiyar commented Sep 27, 2023

mickmister commented Sep 27, 2023

Support for multiple channels #75

Support for multiple channels #75

Conversation

limod commented Mar 28, 2023

mattermost-build commented Mar 28, 2023

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

mickmister Mar 28, 2023

Choose a reason for hiding this comment

mickmister Mar 28, 2023

Choose a reason for hiding this comment

limod commented Mar 29, 2023

stylianosrigas left a comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

mattermost-build commented Apr 9, 2023

limod commented May 30, 2023

mickmister commented Jul 6, 2023

jasonblais commented Jul 7, 2023

Kshitij-Katiyar commented Sep 27, 2023

mickmister commented Sep 27, 2023

codecov bot commented Mar 28, 2023 •

edited

Loading