-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
pkg/api/cortex-ruler.go
Outdated
@@ -186,11 +203,52 @@ func (c *GettableRuleGroupConfig) validate() error { | |||
return nil | |||
} | |||
|
|||
// ApiDuration extends model.Duration | |||
// for handling JSON serialization/deserialization | |||
type ApiDuration model.Duration |
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.
type ApiDuration model.Duration | |
type Duration model.Duration |
This ia already part of the api package, I believe it's redundant.
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.
sorry, I don't fully understand; why it's redundant?
} | ||
return GrafanaManagedRule | ||
|
||
if n.GrafanaManagedAlert != nil { |
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.
I'm not sure this validation belong in Type
, is there any particular reason for it? Otherwise, I'd be inclined for following the pattern throughout this file and creating a function called validate
that we can use as we unmarshal.
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.
The only reason is that Type() and validate() will end up very similar but I will change it.
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.
Once you've addressed @gotjosh's feedback, LGTM
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.
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.
LGTM.
pkg/api/cortex-ruler.go
Outdated
@@ -96,7 +96,8 @@ type NamespaceConfigResponse map[string][]GettableRuleGroupConfig | |||
|
|||
// swagger:model | |||
type PostableRuleGroupConfig struct { | |||
Name string `yaml:"name" json:"name"` | |||
Name string `yaml:"name" json:"name"` | |||
// Example 1m |
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.
I'm not sure we need these changes here. The same applies for the RuleNode down below.
GettableGrafanaRule
I had to:
extend model.Duration type to implement theThis is finally not required (JSON unmarshalling works without it) so I have revert it.json.Marshaler
andjson.Unmarshaler
interfaces