feat: add support mention slack groups for NotificationReceiverSlack and NotificationMention

[hungran]

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

[khanhtc1202]

Ping @minhquang4334 👋

[hungran]

build failed? maybe needed pkg/model/notificationevent.pb.go @minhquang4334

[Warashi]

@hungran

build failed? maybe needed pkg/model/notificationevent.pb.go

Please run make gen/code? This command generates needed files.

[hungran]

@Warashi seem a lot of duplicate code here for handle slackGroups, do you have any idea? @minhquang4334 @khanhtc1202 @Warashi

[Warashi]

I checked the behavior, and this PR worked as expected! Thank you!

By the way, I have an idea about duplicated codes. How about defining the wrapper codes for FindSlackUsers and FindSlackGroups?

I think the codes are below, and I think these codes are defined in the config package.

type Getter interface {
	Get(string) (string, bool)
}

func GetApplicationNotificationMentions(getter Getter, event model.NotificationEventType) (users []string, groups []string, error) {
	n, ok := getter.Get(model.MetadataKeyDeploymentNotification)
	if !ok {
		return []string{}, nil
	}

	var notification DeploymentNotification
	if err := json.Unmarshal([]byte(n), &notification); err != nil {
		return nil, fmt.Errorf("could not extract mentions config: %w", err)
	}

	return notification.FindSlackUsers(event), notification.FindSlackGroups(event), nil
}

WDYT? @hungran @khanhtc1202 @ffjlabo @t-kikuc @minhquang4334

[hungran]

I checked the behavior, and this PR worked as expected! Thank you!

By the way, I have an idea about duplicated codes. How about defining the wrapper codes for FindSlackUsers and FindSlackGroups?

I think the codes are below, and I think these codes are defined in the config package.

type Getter interface {
	Get(string) (string, bool)
}

func GetApplicationNotificationMentions(getter Getter, event model.NotificationEventType) (users []string, groups []string, error) {
	n, ok := getter.Get(model.MetadataKeyDeploymentNotification)
	if !ok {
		return []string{}, nil
	}

	var notification DeploymentNotification
	if err := json.Unmarshal([]byte(n), &notification); err != nil {
		return nil, fmt.Errorf("could not extract mentions config: %w", err)
	}

	return notification.FindSlackUsers(event), notification.FindSlackGroups(event), nil
}

WDYT? @hungran @khanhtc1202 @ffjlabo @t-kikuc @minhquang4334

@Warashi thank you very much for checking, for me avoid duplicate code in this case is greats, but better to see more view point, advice from team

[ffjlabo]

Thank you for asking.

@Warashi 's idea seems to have two purposes.

• to unify the process to get users and groups in the same method
• to unify the above processes on the scheduler, planner, and executor in the same method

I totally agree with the idea, but I am concerned about the second purpose. For me, creating the interface Getter is a little bit overly complex because it only abstracts the metadataStore.Get.

So how about solving the first one at the moment? It means to create getApplicationNotificationMentions on the scheduler, planner, executor like this.

WDYT? @hungran @Warashi @khanhtc1202 @t-kikuc @minhquang4334

func (s *scheduler) getApplicationNotificationMentions(n *DeploymentNotification , event model.NotificationEventType) ([]string, []string, error) {
  n, ok := s.metadataStore.Shared().Get(model.MetadataKeyDeploymentNotification)
  if !ok {
    return []string{}, nil
  }
  var notification config.DeploymentNotification
  if err := json.Unmarshal([]byte(n), &notification); err != nil {
    return nil, fmt.Errorf("could not extract mentions users config: %w", err)
  }

  return notification.FindSlackUsers(event), notification.FindSlackGroups(event), nil
}

func (p *planner) getApplicationNotificationMentions(n *DeploymentNotification , event model.NotificationEventType) ([]string, []string, error) {
  n, ok := s.metadataStore.Shared().Get(model.MetadataKeyDeploymentNotification)
  if !ok {
    return []string{}, nil
  }
  var notification config.DeploymentNotification
  if err := json.Unmarshal([]byte(n), &notification); err != nil {
    return nil, fmt.Errorf("could not extract mentions users config: %w", err)
  }

  return notification.FindSlackUsers(event), notification.FindSlackGroups(event), nil
}

func (e *Executor) getApplicationNotificationMentions(n *DeploymentNotification , event model.NotificationEventType) ([]string, []string, error) {
  n, ok := s.metadataStore.Shared().Get(model.MetadataKeyDeploymentNotification)
  if !ok {
    return []string{}, nil
  }
  var notification config.DeploymentNotification
  if err := json.Unmarshal([]byte(n), &notification); err != nil {
    return nil, fmt.Errorf("could not extract mentions users config: %w", err)
  }

  return notification.FindSlackUsers(event), notification.FindSlackGroups(event), nil
}

[Warashi]

For me, creating the interface Getter is a little bit overly complex because it only abstracts the metadataStore.Get.

I agree with you.

I found metadatastore.Getter interface is already defined. How about using this instead of defining another one? https://github.com/pipe-cd/pipecd/blob/6400c898654e41ac80b6c2ed5a345a8b0bc080a7/pkg/app/piped/metadatastore/store.go#L27-L30

[ffjlabo]

@Warashi I agree with using the interface. Also, I got another concern 🙏

I think there are two processes in the GetApplicationNotificationMentions.

• get the value (type *config.DeploymentNotification) from the metadataStore
• get the users and groups from *config.DeploymentNotification

For the first one, it would be nice to separate it as an another component which has the responsibility to get the proper values from metadataStore. I think it is simpler.

But we should consider it on the other issue. So we focus on fixing to unify only getMentionedUsers and getMentionedGroups to the getApplicationNotificationMentions on the scheduler, planner, executor.

WDYT?

[Warashi]

@ffjlabo I agree. Let's start by unifying only getMentionedUsers and getMentionedGroups.

[hungran]

@ffjlabo @Warashi Thank you so much for your feedback

@ffjlabo

I think there are two processes in the GetApplicationNotificationMentions.

• get the value (type *config.DeploymentNotification) from the metadataStore
• get the users and groups from *config.DeploymentNotification

agreed unify the vaule (type *config.DeploymentNotification) from the metadataStore should be unify by another component/function but here im clear how to continue with the way to focus on fixing getMentionedUsers and getMentionedGroups

[hungran]

@ffjlabo @Warashi thank you for your feedback, updated to follow the idea on unify process to get Users and Groups for planner, scheduler, executor