-
Notifications
You must be signed in to change notification settings - Fork 111
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
pipeline: don't send Slack update for dev pipelines #326
pipeline: don't send Slack update for dev pipelines #326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that condition work inside the utility function itself so it doesn't need to be duplicated in all files?
I think it should. I'll rework the PR. |
56a88a9
to
59a605c
Compare
Much easier this way. ⬆️ Thanks for the suggestion @dm0- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but it will still write some comments to the console log for build status changes that weren't important enough for a Slack notification. For consistency, maybe those should be skipped as well (e.g. by adding || params.DRY_RUN
to the first condition in the function that returns), but it's minor enough that it probably won't matter.
59a605c
to
df5e1c6
Compare
@dm0- Yeah, you are right. I was too focused on the actual sending rather than the function as a whole. Pushed another update ⬆️ |
/lgtm |
/retest |
No description provided.