adds name, value, SlackConfirmationField to slack action #1557

auhlig · 2018-09-20T15:42:49Z

Fixes #1556. Either URL or Name and Value need to be provided in slack action. LGTY @simonpasquier? Thanks in advance

simonpasquier · 2018-09-21T14:19:25Z

Hmm, I'm definitely baffled by the lack of consistency in the Stack documentation. https://api.slack.com/docs/interactive-message-field-guide#action_fields says that name, text and type are mandatory while https://api.slack.com/docs/message-attachments#action_fields doesn't mention name and states that url is mandatory. I'd be reluctant to merge a change without more insights on the right way.

@stuartnelson3 any thoughts on this?

auhlig · 2018-09-21T15:32:08Z

I agree the slack API is quite confusing.
I understood, there are (at least) 2 types of buttons in slack: message and link buttons in a slack action.
The API says type, text, url are ..required when using link buttons (https://api.slack.com/docs/message-attachments#action_fields).
While message buttons as specified here need to have name, text, type.

simonpasquier

You would need to add a test to notifiers_test.go similar to

alertmanager/config/notifiers_test.go

Line 325 in 3558e6f

func TestSlackFieldConfigValidation(t *testing.T) {

simonpasquier · 2018-09-28T07:58:24Z

config/notifiers.go

@@ -252,9 +255,13 @@ func (c *SlackAction) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	if c.Text == "" {
 		return fmt.Errorf("missing value in Slack text configuration")
 	}
-	if c.URL == "" {
+	// either URL or Name and Value need to be provided
+	if c.URL != "" && c.Name != "" && c.Value != "" {


Based on your last comment, the code should be:

if c.URL == "" && c.Name == "" { return fmt.Errorf("missing 'name' or 'url' in Slack action configuration") }

right. adjusted

simonpasquier · 2018-09-28T07:59:37Z

config/notifiers.go

@@ -252,9 +255,13 @@ func (c *SlackAction) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	if c.Text == "" {
 		return fmt.Errorf("missing value in Slack text configuration")


For consistency with L250, this should read "missing 'text' in Slack action configuration"

auhlig · 2018-10-02T19:24:13Z

Done @simonpasquier.
I also added the confirmation fields to the slack action.

simonpasquier

Thanks for the effort! You would need to extend the unit tests too, see there:

alertmanager/config/notifiers_test.go

Line 325 in 30aae38

func TestSlackFieldConfigValidation(t *testing.T) {

simonpasquier · 2018-10-03T12:27:44Z

config/notifiers.go

@@ -232,12 +232,16 @@ func (c *PagerdutyConfig) UnmarshalYAML(unmarshal func(interface{}) error) error

 // SlackAction configures a single Slack action that is sent with each notification.
 // Each action must contain a type, text, and url.
-// See https://api.slack.com/docs/message-attachments#action_fields for more information.
+// See https://api.slack.com/docs/message-attachments#action_fields and https://api.slack.com/docs/message-buttons
+// for more information


(nit) missing dot at the end

simonpasquier · 2018-10-03T12:28:35Z

config/notifiers.go

 	}
-	if c.URL == "" {
-		return fmt.Errorf("missing value in Slack url configuration")
+	// either URL or Name and Value need to be provided


missing dot

simonpasquier · 2018-10-03T12:42:35Z

config/notifiers.go

 	}
+	if (c.Name == "" || c.Value == "") && c.URL == "" {


I'm still not sure that it is the correct approach: AFAIU value isn't mandatory. Also what happens if the configuration defines url and name at the same time? What is the result in Slack?

Maybe do something like this instead?

if c.URL != "" { // Clear all message action fields. c.Name = "" c.Value = "" c.ConfirmField = nil } else if c.Name != "" { c.URL = "" } else { return fmt.Errorf("missing name or url in Slack action configuration") }

Using both url, name seems to be no error to slack. However, your approach ensures we're not having both as per API spec. Using that

simonpasquier · 2018-10-03T12:42:56Z

config/notifiers.go

+	DismissText string `yaml:"dismiss_text,omitempty"  json:"dismiss_text,omitempty"`
+}
+
+// UnmarshalYAML implements the yaml.Unmarshaler interface for SlackConfirmationField


missing dot

config/notifiers.go

auhlig · 2018-10-04T19:16:15Z

Also extended tests. LGTY @simonpasquier?

auhlig · 2018-10-08T09:29:41Z

Anything else that needs to be done @simonpasquier?

simonpasquier

ConfirmField is declared in the configuration but not used by the receiver AFAICT

simonpasquier · 2018-10-08T09:46:28Z

config/notifiers.go

@@ -250,10 +254,39 @@ func (c *SlackAction) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return fmt.Errorf("missing type in Slack action configuration")
 	}
 	if c.Text == "" {
-		return fmt.Errorf("missing value in Slack text configuration")
+		return fmt.Errorf("missing text value in Slack text configuration")


"missing text in Slack action configuration"

simonpasquier · 2018-10-08T09:48:02Z

config/notifiers.go

+		return err
+	}
+	if c.Text == "" {
+		return fmt.Errorf("missing text in slack action confirm")


"missing text in slack configuration configuration"

notify/impl.go

simonpasquier

LGTM though SlackConfirmationField fields could be templated too for consistency.

auhlig · 2018-10-14T00:10:09Z

uses tmplText() on SlackConfirmationField. LGTY @simonpasquier?

auhlig · 2018-10-16T21:23:34Z

Thanks a lot for all the helpful guidance!
Anything else I need to do on this @simonpasquier?

simonpasquier · 2018-10-17T07:59:56Z

Thanks for your patience @auhlig. I'll give it a try as I happen to have access to a Slack instance...
Once it is merged, the documentation would need updates too.

simonpasquier · 2018-10-17T08:51:13Z

notify/impl.go

@@ -747,6 +747,14 @@ func (n *Slack) Notify(ctx context.Context, as ...*types.Alert) (bool, error) {
 				Text:  tmplText(action.Text),
 				URL:   tmplText(action.URL),
 				Style: tmplText(action.Style),
+				Name:  tmplText(action.Name),
+				Value: tmplText(action.Value),
+				ConfirmField: &config.SlackConfirmationField{


You need to test if action.ConfirmField is nil otherwise it will panic.

auhlig · 2018-10-18T12:12:18Z

Added check and opened PR for docs @simonpasquier

simonpasquier

👍

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig · 2018-10-19T18:21:49Z

Rebased after conflicts.

Did slack attachments ever work @simonpasquier? While trying this I slack told me the callback_id for attachments is mandatory: https://api.slack.com/docs/interactive-message-field-guide#attachment_fields
Anyway works perfectly now

auhlig · 2018-10-19T19:33:10Z

Not sure why (only) travis complains. Do I need to run make assets here?
Works on my mac.

auhlig · 2018-10-22T08:30:36Z

Tested with slack. Works for me (TM).
Will update prometheus/docs#1196 today.
Would you mind taking another look @simonpasquier?

auhlig · 2018-10-22T08:38:19Z

Tested with slack. Works for me (TM).
Will update prometheus/docs#1196 today.
Would you mind taking another look and consider merging @simonpasquier? ❤️

auhlig · 2018-10-22T08:38:49Z

Tested with slack. Works for me (TM).
Will update prometheus/docs#1196 today.
Would you mind taking another look and consider merging @simonpasquier? ❤️

simonpasquier · 2018-10-22T13:31:49Z

Did slack attachments ever work?

They surely did. TBH Slack is terrible at dealing with backward compatibility: things that used to work at time T may break at time T+N for some users and continue to work for the others. I wouldn't include f61c50d in this PR as it is unrelated. If it is broken with the current master, please file an issue with all relevant details and it should be addressed separately.

auhlig · 2018-10-22T16:32:30Z

Ewww slack -.-
Extracted the callback_id to #1592 and reset this PR to the last approved commit. Sorry for the back- and forth. Is this good to go @simonpasquier?

simonpasquier

One last nit and this is ok for me.

simonpasquier · 2018-10-23T08:15:59Z

config/notifiers.go

@@ -247,12 +247,16 @@ func (c *PagerdutyConfig) UnmarshalYAML(unmarshal func(interface{}) error) error

 // SlackAction configures a single Slack action that is sent with each notification.
 // Each action must contain a type, text, and url.


Ah the comment is outdated. Just remove it please.

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

simonpasquier · 2018-10-23T09:12:34Z

👍 cc @mxinden @stuartnelson3

auhlig · 2018-10-24T16:25:39Z

Not pushing. Just want to know whether anything else needs to be done to get this merged? @mxinden @stuartnelson3

mxinden

Thanks for the contribution and bearing with us!

simonpasquier · 2018-10-29T14:58:34Z

Yes thanks for the work @auhlig. That was more involved than expected!

auhlig · 2018-10-29T19:50:44Z

Thanks guys ❤️

auhlig force-pushed the slack_message_buttons branch from b63cf76 to b3e5e16 Compare September 20, 2018 15:45

simonpasquier requested changes Sep 28, 2018

View reviewed changes

auhlig force-pushed the slack_message_buttons branch from 1ebb855 to 56c79a0 Compare October 2, 2018 19:19

simonpasquier requested changes Oct 3, 2018

View reviewed changes

simonpasquier reviewed Oct 8, 2018

View reviewed changes

simonpasquier reviewed Oct 9, 2018

View reviewed changes

auhlig changed the title ~~adds name, value to slack action~~ adds name, value, SlackConfirmationField to slack action Oct 16, 2018

simonpasquier reviewed Oct 17, 2018

View reviewed changes

auhlig mentioned this pull request Oct 18, 2018

[slack] adds config for name, value, SlackActionConfirmField prometheus/docs#1196

Merged

simonpasquier approved these changes Oct 18, 2018

View reviewed changes

auhlig added 8 commits October 19, 2018 20:18

adds name, value to slack action

a47a28a

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

incorporate comments

0beb45b

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

adds confirmation field to slack action

1e2fe7e

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

address comments

17b30a3

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

extends tests

0fa65e8

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

fix error message. slack configuration

9525fdb

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

SlackConfirmationField tmplText

6f0859d

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

only add SlackConfirmationField != nil to SlackAction

958197a

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig force-pushed the slack_message_buttons branch from c073c3c to 8a4f363 Compare October 19, 2018 18:18

auhlig force-pushed the slack_message_buttons branch from 1e7a462 to ac52517 Compare October 19, 2018 18:31

auhlig force-pushed the slack_message_buttons branch from ac52517 to f61c50d Compare October 19, 2018 19:02

auhlig force-pushed the slack_message_buttons branch from db2cdc3 to 958197a Compare October 22, 2018 16:21

simonpasquier reviewed Oct 23, 2018

View reviewed changes

remove outdated comment

7c2a5b1

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

mxinden approved these changes Oct 29, 2018

View reviewed changes

mxinden merged commit b63b560 into prometheus:master Oct 29, 2018

auhlig deleted the slack_message_buttons branch October 29, 2018 19:50

		@@ -252,9 +255,13 @@ func (c *SlackAction) UnmarshalYAML(unmarshal func(interface{}) error) error {
		if c.Text == "" {
		return fmt.Errorf("missing value in Slack text configuration")

		@@ -247,12 +247,16 @@ func (c *PagerdutyConfig) UnmarshalYAML(unmarshal func(interface{}) error) error

		// SlackAction configures a single Slack action that is sent with each notification.
		// Each action must contain a type, text, and url.

adds name, value, SlackConfirmationField to slack action #1557

adds name, value, SlackConfirmationField to slack action #1557

Conversation

auhlig commented Sep 20, 2018

simonpasquier commented Sep 21, 2018

auhlig commented Sep 21, 2018

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig commented Oct 2, 2018

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig commented Oct 4, 2018

auhlig commented Oct 8, 2018

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

auhlig commented Oct 14, 2018

auhlig commented Oct 16, 2018

simonpasquier commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auhlig commented Oct 18, 2018

simonpasquier left a comment

Choose a reason for hiding this comment

auhlig commented Oct 19, 2018 • edited Loading

auhlig commented Oct 19, 2018 • edited Loading

auhlig commented Oct 22, 2018

auhlig commented Oct 22, 2018

auhlig commented Oct 22, 2018

simonpasquier commented Oct 22, 2018

auhlig commented Oct 22, 2018

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Oct 23, 2018

auhlig commented Oct 24, 2018 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

simonpasquier commented Oct 29, 2018

auhlig commented Oct 29, 2018

auhlig commented Oct 19, 2018 •

edited

Loading

auhlig commented Oct 19, 2018 •

edited

Loading

auhlig commented Oct 24, 2018 •

edited

Loading