remove a validation check in notification-mention #5155
Conversation
Signed-off-by: nnnkkk7 <kurodanaoki0711pana@gmail.com>
nnnkkk7
requested review from
nghialv,
khanhtc1202,
kentakozuka,
ffjlabo,
t-kikuc and
Warashi
as code owners
nnnkkk7
changed the title
| @@ -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 { | |||
There was a problem hiding this comment.
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 👀
There was a problem hiding this comment.
ok
but when we don't need Slack, SlackGroups or SlackUsers settings, this check return err and we can't deploy by pipecd...
There was a problem hiding this comment.
can you share the configuration which failed the validation test in your usecase?
There was a problem hiding this comment.
@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
↓
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
〜
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes.
Exactly right.
There was a problem hiding this comment.
@nnnkkk7 Thank you for clarifying. I approved this change after discuss with the team 👍
nnnkkk7
changed the title
t-kikuc
changed the title
What this PR does / why we need it:
This PR removes an validation check in the Validate method of the NotificationMention struct.
This contradicts our documentation( https://pipecd.dev/docs-v0.48.x/user-guide/configuration-reference/#notificationmention ), which states these fields are optional, and the omitempty JSON tags on the struct fields.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: No