sensu-plugins / sensu-plugins-slack

Sensu Slack chat handlers
http://sensu-plugins.io
MIT License
30 stars 54 forks source link

message_template ignored if check has a notification #70

Open stewartwebb opened 5 years ago

stewartwebb commented 5 years ago

I'm trying to use a message_template. On upgrade from 1.0.0 to 1.1.1, some of my slack messages were "truncated". It was only showing the notification. I investigated and I think it is due to a fix in 1.1.0: https://github.com/sensu-plugins/sensu-plugins-slack/pull/25. I set the notification field in some my checks and in the event where it is set the handler is ignoring my message_template in favour of the raw notification. I think the fix was correct but I'm not sure if the behaviour is correct?

The line that I think is suspicious.

https://github.com/dunpyl/sensu-plugins-slack/blob/c527e90c319320c69dc786b8a7204b0c9da736fe/bin/handler-slack.rb#L89-L96

How the logic should work in my head is something like this:


if payload_template {
  // use that
} else {
  if message_template {
    // Use message_template
  } else if @event['check']['notification'] {
    // Use notification
  } else {
    // Construct with generated bits
  }
}
majormoses commented 5 years ago

I think that makes sense, this would technically be a breaking change and would need to be done as a major release; which is perfectly ok we just want to avoid surprises for people in minor release versions. We follow semver and a major vs a minor bump indicates nothing more than there was a breaking change. It is meant to convey the safety of upgrades to the consumer. I would try to look at making the changes look something like this: