feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention #4903

hungran · 2024-04-30T07:19:47Z

What this PR does / why we need it:
as mentioned from the issue #4730, currently PipeCD pipeCD is supporting individual user mention

According on difference syntax that slack support mention in message

user
group

This PR will have feature to add field mentionedGroups for NotificationReceiverSlack and slackGroups for NotificationMention

Which issue(s) this PR fixes:

Fixes #4730

Does this PR introduce a user-facing change?: N/A

How are users affected by this change: N/A
Is this breaking change: N/A
How to migrate (if breaking change): N/A

t-kikuc · 2024-04-30T23:48:11Z

pkg/model/notificationevent.pb.go

As the comment in the file says as following,

// Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.27.1 // protoc v3.18.1 // source: pkg/model/notificationevent.proto

you need to edit notificationevent.proto instead and run make gen/code to generate protobuf files.

Thanks @t-kikuc i reverted the change notificationevent.pb.go and successfully ran make gen/code

@t-kikuc
correct me if im wrong
instead change notificationevent.pb.go, we add field at pkg/model/notificationevent.proto ?

…ackGroups for NotificationMention Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

t-kikuc

Thank you for your great contribution!

t-kikuc · 2024-05-07T01:49:33Z

docs/content/en/docs-v0.47.x/user-guide/configuration-reference.md

These changes should be removed.

docs/content/en/docs-v0.47.x/user-guide/configuration-reference.md

docs/content/en/docs-v0.47.x/user-guide/managing-piped/configuration-reference.md

docs/content/en/docs-v0.47.x/user-guide/managing-piped/configuring-notifications.md

That's because this feature would be released in v0.48.0 or v0.48.0-rcX, not v0.47.x

t-kikuc · 2024-05-07T02:39:25Z

pkg/app/pipedv1/notifier/slack.go

It seems you need to copy the changes of notifier/slack.go & slack_test.go to piped/notifier/ 🙇

t-kikuc · 2024-05-07T02:42:01Z

pkg/app/pipedv1/notifier/slack.go

 		title = fmt.Sprintf("Deployment for %q was planned", md.Deployment.ApplicationName)
 		text = md.Summary
-		generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts))
+		generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts), getAccountsAsString(md.MentionedGroups))


You mean this??

Suggested change

generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts), getAccountsAsString(md.MentionedGroups))

generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts), getGroupsAsString(md.MentionedGroups))

sorry a typo, fixed

t-kikuc · 2024-05-07T02:47:28Z

pkg/app/pipedv1/notifier/slack.go

@@ -205,20 +205,21 @@ func (s *slack) buildSlackMessage(event model.NotificationEvent, webURL string)
 		fields            []slackField
 	)

-	generateDeploymentEventData := func(d *model.Deployment, accounts string) {
+	generateDeploymentEventData := func(d *model.Deployment, accounts string, groups string) {


[IMO]
How about calling getAccountsAsString(), getGroupsAsString() here to simplify the switch part?

Suggested change

generateDeploymentEventData := func(d *model.Deployment, accounts string, groups string) {

generateDeploymentEventData := func(d *model.Deployment, accounts, groups []string) {

accountsStr := getAccountsAsString(accounts)

groupsStr := getGroupsAsString(groups)

t-kikuc · 2024-05-07T02:48:04Z

pkg/app/pipedv1/notifier/slack.go

 			{"Started At", makeSlackDate(d.CreatedAt), true},
 		}
 	}

-	generateDeploymentEventDataForTriggerFailed := func(app *model.Application, hash, msg, accounts string) {
+	generateDeploymentEventDataForTriggerFailed := func(app *model.Application, hash, msg, accounts string, groups string) {


[IMO]
How about calling getAccountsAsString(), getGroupsAsString() here to simplify the switch part?

Suggested change

generateDeploymentEventDataForTriggerFailed := func(app *model.Application, hash, msg, accounts string, groups string) {

generateDeploymentEventDataForTriggerFailed := func(app *model.Application, hash, msg string, accounts, groups []string) {

accountsStr := getAccountsAsString(accounts)

groupsStr := getGroupsAsString(groups)

@t-kikuc thank you, yes, calling that way should be visible!

ffjlabo · 2024-05-07T10:29:00Z

pkg/config/piped.go

+	mentionedGroups := make([]string, 0, len(n.MentionedGroups))
+	for _, mentionedGroup := range n.MentionedGroups {
+		formatMentionedGroup := fmt.Sprintf("<!subteam^%s>", strings.TrimPrefix(mentionedGroup, "@"))
+		mentionedGroups = append(mentionedGroups, formatMentionedGroup)
+	}


Is this logic necessary here? It seems also in the func getGroupsAsString 👀
If possible, it would be nice to put it in the getGroupsAsString because this func is just for validating. :)

um not sure but i've seen we already have same thing for user

mentionedAccounts := make([]string, 0, len(n.MentionedAccounts)) for _, mentionedAccount := range n.MentionedAccounts { formatMentionedAccount := strings.TrimPrefix(mentionedAccount, "@") mentionedAccounts = append(mentionedAccounts, formatMentionedAccount) }

should we already remove it as well because getAccountsAsString already have it?

khanhtc1202 · 2024-05-10T07:34:03Z

Ping @minhquang4334 👋

minhquang4334 · 2024-05-11T03:26:42Z

docs/content/en/docs-dev/user-guide/managing-piped/configuring-notifications.md

+            - '@user2'
+          mentionedGroups:
+            - 'group1'
+            - '@group2'


IMO, we should not use "@" to mention a slack group because the handle field after '@' (eg. group2) is mutable, it is not convenient since changing the handle involves updating configuration.
refs: https://slack.com/help/articles/212906697-Create-a-user-group

Thus, I suggest using <!subteam^ID> for group mentioning, since the fields are immutable and can be used according to Slack's official docs.

@minhquang4334 thank you very much for your support, yes I agreed with your point
so should be support both <!subteam^ID> or only ID

@minhquang4334 just from the time being, as slack's client, using slack to mention user and group has exactly same way (by @) so as i understand, this field is for human interactive, we should have same way with Slack UI. Just though

btw, i followed your idea :)

thank you @hungran

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

…ange Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

minhquang4334 · 2024-05-13T03:02:30Z

docs/content/en/docs-dev/user-guide/managing-piped/configuring-notifications.md

+            - 'group1'
+            - '<!subteam^group2>'


Using the groupID or teamID in the documents makes it easier for users to use this feature.

Suggested change

- 'group1'

- '<!subteam^group2>'

- 'groupID1'

- '<!subteam^groupID2>'

minhquang4334 · 2024-05-13T03:03:21Z

docs/content/en/docs-dev/user-guide/managing-piped/configuring-notifications.md

+            - '@user2'
+          mentionedGroups:
+            - 'group1'
+            - '@group2'


thank you @hungran

minhquang4334 · 2024-05-13T03:08:04Z

pkg/config/application.go

 	as := make(map[string]struct{})
 	for _, m := range n.Mentions {
 		if m.Event != allEventsSymbol && "EVENT_"+m.Event != event.String() {
 			continue
 		}
-		for _, s := range m.Slack {
-			as[s] = struct{}{}
+		if len(m.Slack) > 0 {


It would be better if we changed Slack to SlackUsers to separate it from SlackGroups.

@minhquang4334 thanks, will have a look

hungran requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners April 30, 2024 07:19

hungran changed the title ~~feat: add support mention slack groups for NotifactionReceiverSlack and NotificationMention~~ feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention Apr 30, 2024

github-actions bot added the area/go label Apr 30, 2024

t-kikuc reviewed Apr 30, 2024

View reviewed changes

feat: add support mentione groups for NotifactionReceiverSlack and sl…

868baa8

…ackGroups for NotificationMention Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

hungran force-pushed the hungran/slackGroups branch from 8f64f00 to 868baa8 Compare May 1, 2024 05:59

hungran requested a review from t-kikuc May 3, 2024 15:31

t-kikuc reviewed May 7, 2024

View reviewed changes

ffjlabo reviewed May 7, 2024

View reviewed changes

minhquang4334 reviewed May 11, 2024

View reviewed changes

hungran added 9 commits May 11, 2024 12:56

remove change in docs v0.47.x

16c58b8

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

update docs

7adc490

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

update logic for having getGroupsAsString()

bbe24d4

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

using <!subteam^ID> for group mentioning

700b1b7

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

typo

8ef2169

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

call both getAccountsAsString and getGroupsAsString

c667569

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

revert change pkg/model/notificationevent.pb.go

f4441ca

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

use same format for group <!subteam^ID>

91acc34

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

same method to format slack group

8e6b8ca

Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

hungran requested review from minhquang4334, ffjlabo and t-kikuc May 11, 2024 09:19

following slack api, !subteam^ is a literal string that should not ch…

1d7f118

…ange Signed-off-by: hungran <26101787+hungran@users.noreply.github.com>

minhquang4334 reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention #4903

feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention #4903

hungran commented Apr 30, 2024 •

edited

t-kikuc Apr 30, 2024

hungran May 1, 2024 •

edited

hungran May 11, 2024

t-kikuc left a comment

t-kikuc May 7, 2024

t-kikuc May 7, 2024

hungran May 12, 2024

t-kikuc May 7, 2024

hungran May 11, 2024

t-kikuc May 7, 2024

t-kikuc May 7, 2024

hungran May 11, 2024

ffjlabo May 7, 2024

hungran May 11, 2024

khanhtc1202 commented May 10, 2024

minhquang4334 May 11, 2024

hungran May 11, 2024 •

edited

hungran May 11, 2024 •

edited

minhquang4334 May 13, 2024

minhquang4334 May 13, 2024

minhquang4334 May 13, 2024

minhquang4334 May 13, 2024

hungran May 24, 2024

	generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts), getAccountsAsString(md.MentionedGroups))
	generateDeploymentEventData(md.Deployment, getAccountsAsString(md.MentionedAccounts), getGroupsAsString(md.MentionedGroups))

-	generateDeploymentEventData := func(d *model.Deployment, accounts string, groups string) {
+	generateDeploymentEventData := func(d *model.Deployment, accounts, groups []string) {
+	accountsStr := getAccountsAsString(accounts)
+	groupsStr := getGroupsAsString(groups)

feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention #4903

Are you sure you want to change the base?

feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention #4903

Conversation

hungran commented Apr 30, 2024 • edited

Choose a reason for hiding this comment

hungran May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kikuc 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

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

khanhtc1202 commented May 10, 2024

Choose a reason for hiding this comment

hungran May 11, 2024 • edited

Choose a reason for hiding this comment

hungran May 11, 2024 • edited

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

hungran commented Apr 30, 2024 •

edited

hungran May 1, 2024 •

edited

hungran May 11, 2024 •

edited

hungran May 11, 2024 •

edited