Fix -ruler.alertmanager-url flag #2989
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was never space separated, you needed to register the flag multiple times for it to append to the list of strings. On top of that, the previous change implied that when using a config file you would need to provide a a list instead of a string thus breaking exisiting configuration. Signed-off-by: gotjosh <[email protected]>
We had two very similar flag extensions: `Strings` and `StringSlice`. With this, we unify its use by removing `Strings` in favour of `StringSlice` but keeping the string representation favoured in `Strings`. The difference here is that by using `Sprintf` we would get the brackets representation as part of the string. Signed-off-by: gotjosh <[email protected]>
This is no longer only about the API. Signed-off-by: gotjosh <[email protected]>
Signed-off-by: gotjosh <[email protected]>
gotjosh
commented
Aug 6, 2020
|
|
||
| // StringSlice is a slice of strings that implements flag.Value | ||
| type StringSlice []string | ||
|
|
||
| // String implements flag.Value | ||
| func (v StringSlice) String() string { | ||
| return strings.Join(v, " ") | ||
| return fmt.Sprintf("%s", []string(v)) |
There was a problem hiding this comment.
The unification of the flag extensions is basically that I kept the name StringSlice but "borrowed" the String casting function.
I like this because for empty values it renders [] instead of "" which seems to be more accurate.
Signed-off-by: gotjosh <[email protected]>
|
I'm not sure what to include in the changelog - opinions are welcome. |
gouthamve
approved these changes
Aug 7, 2020
Nothing. I don't see any user-impact since the last release that is introduced by this PR. |
pracucci
approved these changes
Aug 7, 2020
pracucci
pushed a commit
to pracucci/cortex
that referenced
this pull request
Aug 7, 2020
* fix: `-ruler.alertmanager-url` should be a string This was never space separated, you needed to register the flag multiple times for it to append to the list of strings. On top of that, the previous change implied that when using a config file you would need to provide a a list instead of a string thus breaking exisiting configuration. Signed-off-by: gotjosh <[email protected]> * Unify the use of StringSlice flags We had two very similar flag extensions: `Strings` and `StringSlice`. With this, we unify its use by removing `Strings` in favour of `StringSlice` but keeping the string representation favoured in `Strings`. The difference here is that by using `Sprintf` we would get the brackets representation as part of the string. Signed-off-by: gotjosh <[email protected]> * Rename `api_ruler_test` to `ruler_test` This is no longer only about the API. Signed-off-by: gotjosh <[email protected]> * Create an integration test for Alertmanager discovery Signed-off-by: gotjosh <[email protected]> * Fix existing test Signed-off-by: gotjosh <[email protected]>
3 tasks
pracucci
added a commit
that referenced
this pull request
Aug 10, 2020
* fix: `-ruler.alertmanager-url` should be a string This was never space separated, you needed to register the flag multiple times for it to append to the list of strings. On top of that, the previous change implied that when using a config file you would need to provide a a list instead of a string thus breaking exisiting configuration. Signed-off-by: gotjosh <[email protected]> * Unify the use of StringSlice flags We had two very similar flag extensions: `Strings` and `StringSlice`. With this, we unify its use by removing `Strings` in favour of `StringSlice` but keeping the string representation favoured in `Strings`. The difference here is that by using `Sprintf` we would get the brackets representation as part of the string. Signed-off-by: gotjosh <[email protected]> * Rename `api_ruler_test` to `ruler_test` This is no longer only about the API. Signed-off-by: gotjosh <[email protected]> * Create an integration test for Alertmanager discovery Signed-off-by: gotjosh <[email protected]> * Fix existing test Signed-off-by: gotjosh <[email protected]> Co-authored-by: gotjosh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
-ruler.alertmanager-urlby reverting it back to using a string (which is meant to be comma-separated) as opposed to a list of strings. As it was this broke compatibility with a file-based configuration where it required a list of strings instead of a single string.StringsSliceandStringsflag extensions.Quoting the commits:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]