Skip to content
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.

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

Add flag to skip notifying user #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattlyons0
Copy link

I think it would be useful to add an environment variable to control whether the attributed user for the commits is directly notified on slack.

A common case may be if one only wants notifications on failures. Another case could be if one is already notifying users in a channel with @{{slack.name}} in the template (currently this causes 2 notifications, one in the channel and one via DMs).

The current behavior is to always notify, the variables have been configured so that this behavior will be the same by default.

Let me know what you think! Thanks.

Controlled via PLUGIN_SUCCESS_SKIP_USER_NOTIFY & 
PLUGIN_FAILURE_SKIP_USER_NOTIFY
@bradrydzewski
Copy link
Member

bradrydzewski commented Jan 30, 2020

A common case may be if one only wants notifications on failures

Drone supports conditional step execution that allows you to execute pipeline steps based on the status of the pipeline. The below example demonstrates how to execute the slack plugin only when the pipeline is failing.

steps:
- name: notify
  image: plugins/slack
  when:
    status: [ failure ]

More details at https://docker-runner.docs.drone.io/configuration/conditions/#by-status

@bradrydzewski
Copy link
Member

bradrydzewski commented Jan 30, 2020

Sorry I thought I was reviewing plugins/slack and realized this was for plugins/slack-blame. I am less familiar with this plugin and my feedback may no longer apply. I will leave the review to those that know this plugin best.

@donny-dont
Copy link
Contributor

@mattlyons0 so I'm kinda torn here so let me ask if this would be a better behavior.

If there is no channel it always notifies the slack user only. If there's a channel it always notifies the channel only.

@mattlyons0
Copy link
Author

mattlyons0 commented Jan 30, 2020

I believe the current behavior is if your template includes a reference to @{{slack.name}} the user will be notified in the channel. If the template does not include that there will be no notification in the channel. In both cases there will be a DM to the user with the template message.

So if we were to change the behavior, would we check if the was a @ reference in the template?

@donny-dont
Copy link
Contributor

There's always a DM if the user is found whether there's a channel specified or not. I could see a case where the user might not be in the channel or something like that. There could be more complicated decisions like if the user is not in the channel and an @ is not being used in the message. Just wondering how magical it should all be.

@mattlyons0
Copy link
Author

Do you think its worth trying to check those cases and only notify via DM otherwise?

I believe we would need more permissions to check the users in a channel API Details. I could also see a case where the user is in a channel but it is muted.

I'm not even sure if we should be considering a DM on the same level as a notification in a channel given you can't ignore a DM but you can easily ignore channels. A benefit of not handling this automatically (adding another environment variable) is some users of the plugin may value not getting notifications at all (be it in a channel or via DM) and I don't think this is currently possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants