Move slack notification into the blt command #1464
Conversation
| if ($failed) { | ||
| $this->yell(sprintf("Update failed for the following sites:\n%s", implode("\n", $failed)), 100, 'red'); | ||
|
|
||
| if (EnvironmentDetector::isAhStageEnv() || EnvironmentDetector::isAhProdEnv()) { |
There was a problem hiding this comment.
Is there a reason you're using these instead of $target_env?
There was a problem hiding this comment.
Not at all. I didn't really think about it. lol
| if ($failed) { | ||
| $this->yell(sprintf("Update failed for the following sites:\n%s", implode("\n", $failed)), 100, 'red'); | ||
|
|
||
| if (EnvironmentDetector::isAhStageEnv() || EnvironmentDetector::isAhProdEnv()) { | ||
| $this->sendSlackNotification("A new deployment has been made to *$target_env* using *$deployed_tag*. At least one site failed updating."); |
There was a problem hiding this comment.
Why not include the failed sites? Would the message be too long if too many sites failed?
There was a problem hiding this comment.
Yeah I was kinda on the fence about it. I figured it might be less noise in Slack. Maybe we can use the number of sites that failed? then it's still short message
There was a problem hiding this comment.
Ooh I like that idea. A count would be useful without creating too much noise.
| $sites = $this->getConfigValue('multisites'); | ||
| $parallel_executions = 10; | ||
| $parallel_executions = (int) getenv(self::UPDATE_PARALLEL_PROCESSES) ?: 10; |
There was a problem hiding this comment.
I don't 100% understand the need to create the UPDATE_PARALLEL_PROCESSES constant here to call out the name of the environment variable.
There was a problem hiding this comment.
* waves hand over it *
To make it look more fancy.
READY FOR REVIEW
Summary
[briefly summarize the changes here]
Need Review By (Date)
['10/30', 'asap', etc.]
Urgency
['low', 'medium', 'high', etc.]
Steps to Test
PR Checklist