Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1096 notification use body switch #1101

Merged
merged 1 commit into from
Dec 16, 2015
Merged

1096 notification use body switch #1101

merged 1 commit into from
Dec 16, 2015

Conversation

ayashjorden
Copy link
Contributor

Hi,
As discussed with @captncraig, and in issue 1096:

  1. When taking action on several alerts, list alert names in bulk message passed to notifiaction.
  2. let . have the .Subject and .Body (as per @mjibson idea)
  3. Bosun main page, let the alert list use the template's subject.

Review on Reviewable

captncraig
captncraig reviewed Jun 25, 2015
View reviewed changes
cmd/bosun/conf/conf.go

next string
email string
post, get string
body string
// Maybe not used, need to learn more GO :(
useBody bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed not used.

@ayashjorden
Copy link
Contributor Author

Hi @captncraig,
If we're going to make template's body available for notification body, will the template's body still be rendered as HTML? I once tried using the template's body to render json and got the json object wrapped with HTML tags.

My pure intention with this PR and issue 1096 was to have a clean template subject and use the template's body to render a json object to be sent to our Hipchat bot.

I've read the docs about 'json' function available in notification body template,(assuming we'll add the template's body to be available in notification body) it is unclear if the template's body will be rendered as HTML and then passed to notification body or notification body will take the template's body as plain text and render it.

@ayashjorden
Copy link
Contributor Author

Running validate locally exited with code 0.

@captncraig
Copy link
Contributor

@ayashjorden sorry this has sat so long. If you can rebase to master and resolve conflicts I can probably merge it. Something about it feels a bit off, but I think it is an ok solution for now.

@ayashjorden ayashjorden force-pushed the 1096_notification_use_body_switch branch from ee51c60 to dcf238e Compare November 25, 2015 09:06
@ayashjorden
Copy link
Contributor Author

@captncraig,
I've updated my branch with latest from bosun.org/bosun:master.

I'd be happy to understand why this solution is a 'bit off' from your point of view...

Thank you,
Yarden

@ayashjorden ayashjorden force-pushed the 1096_notification_use_body_switch branch from f34df42 to c9b7718 Compare December 16, 2015 21:01
Add useBody switch to notification element
f3b1340 
    Enables the notification to render the template's body content instead of
    subject content.
@ayashjorden ayashjorden force-pushed the 1096_notification_use_body_switch branch from c9b7718 to f3b1340 Compare December 16, 2015 22:08
@captncraig captncraig merged commit f3b1340 into bosun-monitor:master Dec 16, 2015
@ayashjorden
Copy link
Contributor Author

@captncraig @kylebrandt ,
defrati referred me to an issue I mentioned earlier(> month), but we didn't conclude on.
The template's body is rendered as HTML, thus that the JSON string is wrapped <html><head></head><body>JSON_STRING_HERE</body></html> .

To overcome this, I've been using a regex ("<html><head><\/head><body>(.*)<\/body><\/html>) to extract the JSON string and parse in the consumer(e.g. Will bot).

What do you think?
Yarden

@deanefrati
Copy link

i would really love to see an option to encode the body as JSON natively. I'm using the same workaround @ayashjorden is using right now but it's not the prettiest way to handle this... Also the ack and close messages are HTML encoded, it would be great if those can also have an option to JSON encode. This will give us a lot of flexibility for alert processing in external tools, for example we are looking at using stackstorm to receive the alerts from Bosun and run some automations based on the alert fields.

@deanefrati deanefrati mentioned this pull request Dec 21, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ayashjorden @captncraig @deanefrati