Skip to content

remove a validation check in notification-mention #5155

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.

Sign up for GitHub

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

Merged
t-kikuc merged 1 commit into from
Aug 29, 2024
Merged

remove a validation check in notification-mention #5155

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions pkg/config/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,6 @@ type NotificationMention struct {
}

func (n *NotificationMention) Validate() error {
if len(n.Slack) == 0 && len(n.SlackGroups) == 0 && len(n.SlackUsers) == 0 {
Copy link
Member

@khanhtc1202 khanhtc1202 Aug 23, 2024

Choose a reason for hiding this comment

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

I think the point of this check is to ensure at least one of the configurations must not be empty. So it's not a no meaning 👀

Copy link
Contributor Author

@nnnkkk7 nnnkkk7 Aug 23, 2024

Choose a reason for hiding this comment

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

ok
but when we don't need Slack, SlackGroups or SlackUsers settings, this check return err and we can't deploy by pipecd...

Copy link
Member

@khanhtc1202 khanhtc1202 Aug 23, 2024

Choose a reason for hiding this comment

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

can you share the configuration which failed the validation test in your usecase?

Copy link
Contributor Author

@nnnkkk7 nnnkkk7 Aug 26, 2024

Choose a reason for hiding this comment

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

@khanhtc1202
ok!
This configuration causes it..

apiVersion: pipecd.dev/v1beta1
kind: LambdaApp
spec:
  name: dev-hoge
  labels:
    env: dev
  notification:
    mentions:
      - event: DEPLOYMENT_SUCCEEDED
      - event: DEPLOYMENT_FAILED
  eventWatcher:
    - matcher:
        name: hoge-update
        labels:
          env: dev
      handler:
        type: GIT_UPDATE
        config:
          replacements:
            - file: function.yaml
              regex: 00000.dkr.ecr.ap-northeast-1.amazonaws.com/hoge:fuga

↓

スクリーンショット 2024-08-26 午前10 35 47

Copy link
Member

@khanhtc1202 khanhtc1202 Aug 26, 2024

Choose a reason for hiding this comment

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

  notification:
    mentions:
      - event: DEPLOYMENT_SUCCEEDED
      - event: DEPLOYMENT_FAILED

@nnnkkk7 Thanks for the detailed configuration 👍 In such a case, where would you want the piped to send a notification to? Without slack mentions, there is no meaning for this notification.mentions config. Or could you share your expected behavior of piped you want in this case?

Copy link
Contributor Author

@nnnkkk7 nnnkkk7 Aug 26, 2024

Choose a reason for hiding this comment

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

@khanhtc1202

I see.
Notification settings in piped-config.yaml are as follows. Is this sufficient for notifications?🤔
I want to send a notification to this channel..

analysisProviders:
〜
notifications:
  receivers:
  - name: receiver
    slack:
      channelID: ""
      hookURL: '******'
      oauthToken: ""
      oauthTokenData: ""
      oauthTokenFile: ""
  routes:
  - events:
    - DEPLOYMENT_FAILED
    labels:
      env: dev
    name: dev
    receiver: receiver
  - events:
    - DEPLOYMENT_APPROVED
    - DEPLOYMENT_WAIT_APPROVAL
    - DEPLOYMENT_SUCCEEDED
    - DEPLOYMENT_FAILED
    labels:
      env: stg
    name: stg
    receiver: receiver
  - events:
    - DEPLOYMENT_APPROVED
    - DEPLOYMENT_WAIT_APPROVAL
    - DEPLOYMENT_ROLLING_BACK
    - DEPLOYMENT_SUCCEEDED
    - DEPLOYMENT_FAILED
    - DEPLOYMENT_CANCELLED
    labels:
      env: prd
    name: prd
    receiver: receiver
〜

Copy link
Member

@khanhtc1202 khanhtc1202 Aug 27, 2024

Choose a reason for hiding this comment

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

Thanks, I get it now. In this case, your expected behavior of piped is just to send the notification in those cases to target channel without mention anyone, is it right?

Copy link
Contributor Author

@nnnkkk7 nnnkkk7 Aug 27, 2024

Choose a reason for hiding this comment

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

Yes.
Exactly right.

Copy link
Member

@khanhtc1202 khanhtc1202 Aug 28, 2024

Choose a reason for hiding this comment

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

@nnnkkk7 Thank you for clarifying. I approved this change after discuss with the team 👍

return fmt.Errorf("slack, slackusers or slackGroups must not be empty")
}
if n.Event == allEventsSymbol {
return nil
}
Expand Down Expand Up @@ -779,7 +776,7 @@ func (dd *DriftDetection) Validate() error {
}

func LoadApplication(repoPath, configRelPath string, appKind model.ApplicationKind) (*GenericApplicationSpec, error) {
var absPath = filepath.Join(repoPath, configRelPath)
absPath := filepath.Join(repoPath, configRelPath)

cfg, err := LoadFromYAML(absPath)
if err != nil {
Expand Down