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.

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

Fix config name inconsistency #1087

Merged

Conversation

josedonizetti
Copy link
Contributor

@josedonizetti josedonizetti commented Nov 8, 2017

This PR renames configuration from 3 different notifiers to make it consistent with others. Basically, global configurations will be named <notifier>_api_url, and local configurations per notifier api_url.

This are breaking changes.

Fixes #680 and #875

@stuartnelson3 @brancz @brian-brazil @juliusv

@brian-brazil
Copy link
Contributor

We've generally followed the name that the provider uses in their docs when it comes to these things, as a user cares about only 1 of these which they already know - not that they're consistent across the alertmanager.

Do these all match up with that?

@brancz
Copy link
Member

brancz commented Nov 10, 2017

I wonder how much it's worth breaking these, as I'm pretty sure a lot of configurations are going to break when people upgrade to these changes.

@josedonizetti
Copy link
Contributor Author

josedonizetti commented Nov 10, 2017

@brian-brazil I've changed the PR to rename only two configs:

@josedonizetti
Copy link
Contributor Author

josedonizetti commented Nov 10, 2017

@brancz Yup, agree that it might not be worth. My idea when I first sent this PR was to have it merged for the version 0.10.0, was considering at the time that the first double number in the version would be a good moment to such a break.

But I totally understand if you think that is not worth, let's just add a note about it on the issues so no other PR is created for it, for now, maybe in a future 1.0 release the break might make more sense.

@brancz
Copy link
Member

brancz commented Nov 10, 2017

in a future 1.0 release

I think this would be more appropriate. Similar breaking re-namings were done from Prometheus 0.x to 1.x.

@brian-brazil
Copy link
Contributor

I don't think that this is a breaking change in practice, as these settings should ~never need to be changed from the defaults by users.

Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

As brian said, these values shouldn't be modified by the users. I'll make a note of it in the next release, though.

@stuartnelson3 stuartnelson3 merged commit 76c15a0 into prometheus:master Nov 11, 2017
@aecolley
Copy link

You lose a karma point for this because my employer's private HipChat server means that this was a setting that most definitely needed to be different from the default.

@stuartnelson3
Copy link
Contributor

Thank you for bringing that to my attention. I was not aware hipchat could be run privately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants