slack multi-config #1833
slack multi-config #1833
Conversation
aanthony1243
force-pushed
the
aa_1696_multislack
branch
from
8c1f15a to
b121020
Compare
aanthony1243
force-pushed
the
aa_1696_multislack
branch
from
8275586 to
a6ed1d1
Compare
pipeline/alert.go
Outdated
| // Example: | ||
| // stream | ||
| // |alert() | ||
| // .slack('opencommunity') |
There was a problem hiding this comment.
This example should be:
.slack()
.workspace('opencommunity')
| @@ -52,3 +57,37 @@ func (c Config) Validate() error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The NewConfig() method is now not sufficient to set defaults.
Because its possible that there are multiple config structs created, the toml parser will create a new one using reflection which will not call the NewConfig method.
The config package we use checks for config structs that implement an ApplyDefaults method and calls it on newly created config structs.
For example:
func (c *Config) ApplyDefaults() {
if c.Username == "" {
c.Username = DefaultUsername
}
}
It would be best to use the ApplyDefaults method within the NewConfig method to ensure they are the same.
services/slack/service.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| client := s.clientValue.Load().(*http.Client) | ||
| client := s.clients[workspace] |
There was a problem hiding this comment.
How do you protect access to the clients dict here?
There was a problem hiding this comment.
It might be worth creating a Workspace struct that contains both a Config and http.Client so you do not have to maintain and protect access to two maps.
services/slack/service.go
Outdated
| @@ -184,6 +226,7 @@ func (s *Service) preparePost(channel, message, username, iconEmoji string, leve | |||
| Mrkdwn_in: []string{"text"}, | |||
| } | |||
| postData := make(map[string]interface{}) | |||
| postData["workspace"] = c.Workspace | |||
There was a problem hiding this comment.
Is this part of the slack API?
|
|
||
| if c.InsecureSkipVerify { | ||
| s.diag.InsecureSkipVerify() | ||
| } |
There was a problem hiding this comment.
We also want this check to be in the Service.Update method.
|
Oh and it looks like there is one more |
…the workspace name.
aanthony1243
force-pushed
the
aa_1696_multislack
branch
from
a85b422 to
c2be03b
Compare
slack now accepts + generates request for multiple configurations.
Required for all non-trivial PRs