-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
rules: fix silly mistakes in the rules API #2957
Conversation
Fixes thanos-io#2938 and is a follow up to thanos-io#2925. Changes: * Copying all fields properly to the group-level as well * Set TZ to UTC explicitly in other `time.Time` fields to avoid a panic * Make sure that `RuleGroup` always has an empty array in the `rules` field because that's customary and that is what the new React UI expects. Manual testing for now. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
It has been marked deprecated for more than 2 months. Remove it finally. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great @GiedriusS :)) two small comments from my side
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> Co-authored-by: Lucas Servén Marín <lserven@gmail.com>
Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> Co-authored-by: Lucas Servén Marín <lserven@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
@@ -73,7 +73,6 @@ message RuleGroup { | |||
google.protobuf.Timestamp last_evaluation = 6 [(gogoproto.jsontag) = "lastEvaluation", (gogoproto.stdtime) = true, (gogoproto.nullable) = false ]; | |||
|
|||
// Thanos specific. | |||
PartialResponseStrategy DeprecatedPartialResponseStrategy = 7 [(gogoproto.jsontag) = "partial_response_strategy"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, you are right ;p
Fixes #2938 and is a follow up to #2925.
Changes:
time.Time
fields to avoid a panic;RuleGroup
always has an empty array in therules
field because that's customary and that is what the new React UI
expects;
DeprecatedPartialResponseStrategy
so that we wouldn'thave essentially two fields in the response of
/api/v1/rules
that couldpotentially show different values because one of them is locked to
always be
WARN
.The new e2e test case tests out the first change written in this list and the last one
is covered by unit tests that were changed to work with these modifications.
Signed-off-by: Giedrius Statkevičius giedriuswork@gmail.com