Conversation
31caabd to
cf65ed4
Compare
e8d049c to
ff168b1
Compare
| rf := func(receiverFilter *regexp.Regexp) func(r *dispatch.Route) bool { | ||
| return func(r *dispatch.Route) bool { | ||
| receiver := r.RouteOpts.Receiver | ||
| if receiverFilter != nil && !receiverFilter.MatchString(receiver) { |
There was a problem hiding this comment.
Question, do we need to use regex here? That means the input params.Receiver contains multiple receivers?
There was a problem hiding this comment.
This is more about support regex search for receivers, i am following convention of receiver filter parameter in the other apis
See https://github.com/prometheus/alertmanager/blob/main/api/v2/api.go#L250-L260
|
|
||
| func validateNextToken(nextToken *string) error { | ||
| if nextToken != nil && *nextToken != "" { | ||
| match, _ := regexp.MatchString("^[a-fA-F0-9]{40}$", *nextToken) |
There was a problem hiding this comment.
Let's compile the regex once and match here?
There was a problem hiding this comment.
removed the nextToken != ""
api/v2/api.go
Outdated
| for _, alertGroup := range ags { | ||
|
|
||
| // Skip the aggregation group if the next token is set and hasn't arrived the nextToken item yet. | ||
| if params.NextToken != nil && *params.NextToken != "" && *params.NextToken >= alertGroup.ID { |
There was a problem hiding this comment.
Can you simplify to if len(params.NextToken) > 0 && *params.NextToken >= alertGroup.ID
There was a problem hiding this comment.
removed *params.NextToken != "" check as it won't be empty string from validateNextToken. I won't remove the params.NextToken != nil as won't len(params.NextToken) result in NPE when params.NextToken is nil?
There was a problem hiding this comment.
Nvm. I thought params.NextToken is a string type from the hints when doing code review on GH. If it is *string I am good.
| return nil | ||
| } | ||
|
|
||
| func validateNextToken(nextToken *string) error { |
There was a problem hiding this comment.
I think we can use string here as param?
There was a problem hiding this comment.
I want to escape the validation when nextToken is nil, kind of want to keep that if into this function rather in main function
| } | ||
|
|
||
| func (ag *aggrGroup) GroupID() string { | ||
| h := sha1.New() |
There was a problem hiding this comment.
Any reason to use sha1? Will xxhash or other libraries works
There was a problem hiding this comment.
only reason of using sha1 is wanting to keep the output of the hash to be short as this is going to be nexttoken for user to use
dispatch/route.go
Outdated
| func (r *Route) ID() string { | ||
| b := strings.Builder{} | ||
|
|
||
| var position *int |
There was a problem hiding this comment.
I prefer to not use pointer... We can use int and initialize it to an invalid value like -1 then we will have less pointer reference operations
80fad23 to
e16034e
Compare
* Add tests to TestMatchers This commit adds a number of tests to TestMatchers that asserts some of the more nuanced behavior when parsing label matchers. Signed-off-by: George Robinson <george.robinson@grafana.com> --------- Signed-off-by: George Robinson <george.robinson@grafana.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
| $ref: '#/responses/BadRequest' | ||
| '500': | ||
| $ref: '#/responses/InternalServerError' | ||
| /alertgroups: |
There was a problem hiding this comment.
Why not make this a flag or a subpath on the existing API?
This naming can be confusing with the existing /alerts/groups.
|
@qinxx108 can you please update the PR? |
Implement the proposal here #3361