-
Notifications
You must be signed in to change notification settings - Fork 804
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
Ruler API and ObjectStore Backend #2269
Ruler API and ObjectStore Backend #2269
Conversation
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
…c checks Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@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.
Nice work.
I'd like to see changes to remove some code duplication in api.go, and unit test or two for escaping namespaces and groups in the request paths, to make sure it's not broken by accident in the future, but overall 👍
Co-Authored-By: Peter Štibraný <peter.stibrany@grafana.com> Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
f0c09ba
to
5d88330
Compare
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.
Nice improvements! There are few missing returns
in methods, my other comments are mostly nitpicking.
"github.com/cortexproject/cortex/pkg/util" | ||
) | ||
|
||
// RegisterRoutes registers the ruler API HTTP routes with the provided Router. | ||
func (r *Ruler) RegisterRoutes(router *mux.Router) { | ||
func (r *Ruler) RegisterRoutes(router *mux.Router, middleware middleware.Interface) { | ||
router = router.UseEncodedPath() |
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.
One possible way to test it would be to have muxVars
global variable, that defaults to mux.Vars
but could be set to your own function in tests.
One way would be injecting them into the ctx of the request that is passed to the function. But I feel like it's more important to have integration tests for this behavior since the interplay with the gorilla router is so important. |
Signed-off-by: Jacob Lisi <jacob.t.lisi@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 for addressing my feedback!
API Changes
This PR adds an API to the ruler service based on this design document (https://docs.google.com/document/d/1gNKiC5L_zWQ4GGgtdhJJ6FnaZXyJJBvdGTUmF0wgEFg/edit?usp=sharing)
Object Storage Changes
chunk.ObjectClient
interfaceIntegration Tests
Which issue(s) this PR fixes:
Related #1547 #1244
Checklist
CHANGELOG.md