Skip to content
This repository has been archived by the owner on May 12, 2022. It is now read-only.

Multiple slack channels #68

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cratermoon
Copy link

Please comment and suggest improvements. The AppSec team would like to post to multiple Slack channels on release, this PR adds support for that use case.

@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #68 into master will decrease coverage by 11.37%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #68       +/-   ##
==========================================
- Coverage    17.2%   5.83%   -11.38%     
==========================================
  Files           3       5        +2     
  Lines         587    1732     +1145     
==========================================
  Hits          101     101               
- Misses        472    1617     +1145     
  Partials       14      14
Impacted Files Coverage Δ
context/context.go 15.78% <ø> (ø) ⬆️
ankh/execute.go 0% <0%> (ø)
ankh/main.go 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94cc341...e6836dd. Read the comment docs.

@cratermoon cratermoon force-pushed the multiple-slack-channels branch from b859da8 to 73403d8 Compare February 19, 2019 23:23
ankh/main.go Outdated
if len(ctx.AnkhConfig.Slack.Channels) > 0 {
ctx.Logger.Debugf("Pinging %d slack channels %+v", len(ctx.AnkhConfig.Slack.Channels), ctx.AnkhConfig.Slack.Channels)
if ctx.Mode == ankh.Rollback {
ctx.SlackDeploymentVersion = "rollback"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

Yes!

ankh/main.go Outdated
@@ -431,8 +431,13 @@ func execute(ctx *ankh.ExecutionContext) {
executeContext(ctx, &rootAnkhFile)
}

if ctx.SlackChannel != "" {
if err := slack.PingSlackChannel(ctx); err != nil {
if len(ctx.AnkhConfig.Slack.Channels) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change requires the channel to be set in the ankh config? This doesn't allow the use-case where a different slack channel is used for different application deploys.

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

ctx.Logger.Infof("--dry-run set so not sending message '%v' to slack channel %v", messageText, ctx.SlackChannel)
for name, id := range channels {
if !ctx.DryRun {
_, _, err = api.PostMessage(id, slack.MsgOptionAttachments(attachment), slack.MsgOptionPostMessageParameters(messageParams))
Copy link
Contributor

Choose a reason for hiding this comment

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

This error is not checked inside the loop so only the last channel error will be returned/checked from this function.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about go conventions for handling multiple possible errors and returning, but this will collect all the messages and return a single error.

Copy link
Author

Choose a reason for hiding this comment

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

Steven E. Newton added 4 commits April 24, 2019 10:50
Per a request from Donald Johnson, added support to ankh for
posting to multiple slack channels. Also added channels as an
array to the slack section in ankh config
@cratermoon cratermoon force-pushed the multiple-slack-channels branch from 73403d8 to e6836dd Compare April 24, 2019 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants