Copy cortex/pkg/ruler package dependency into Loki
#5089
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
DylanGuedes
force-pushed
the
fork-cortex-ruler
branch
from
b2df5ff
to
866532c
Compare
DylanGuedes
changed the title
cortex/pkg/ruler package dependency into Loki
|
I recommend reviewing this commit-by-commit. |
jeschkies
approved these changes
Jan 11, 2022
cyriltovena
reviewed
Jan 11, 2022
| @@ -8,15 +8,15 @@ import ( | |||
| "io/ioutil" | |||
| "strings" | |||
|
|
|||
| "github.com/cortexproject/cortex/pkg/chunk" | |||
There was a problem hiding this comment.
Replace with the Loki one.
There was a problem hiding this comment.
good catch. pushed a few commits changing it, wdyt?
cyriltovena
reviewed
Jan 12, 2022
cyriltovena
reviewed
Jan 12, 2022
| @@ -1,4 +1,4 @@ | |||
| package ruler | |||
| package base | |||
|
|
|||
| import ( | |||
| "github.com/prometheus/client_golang/prometheus" | |||
There was a problem hiding this comment.
The util package below could be avoided ?
There was a problem hiding this comment.
Nope, unfortunately. However, we are already working on forking util.
cyriltovena
reviewed
Jan 12, 2022
| @@ -1,4 +1,4 @@ | |||
| package ruler | |||
| package base | |||
|
|
|||
| import ( | |||
| "context" | |||
There was a problem hiding this comment.
Same for the util package of cortex below ?
cyriltovena
reviewed
Jan 12, 2022
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/cortexproject/cortex/pkg/util" |
There was a problem hiding this comment.
same we should get those function in loki.
gotjosh
added a commit
that referenced
this pull request
Feb 10, 2022
We copied over the ruler code as part of #5089, and it seems that at the same time we stopped depending on the object storage implementation of Cortex. Turns out, there are (slight) differences between the clients which meant that response returned by the API when we try to get a rule group that is not found had changed. This ensures that is consistent with the error assertion that we have in the code for the ruler.
3 tasks
dannykopping
pushed a commit
that referenced
this pull request
Feb 10, 2022
* Ruler: Rule group not found API message We copied over the ruler code as part of #5089, and it seems that at the same time we stopped depending on the object storage implementation of Cortex. Turns out, there are (slight) differences between the clients which meant that response returned by the API when we try to get a rule group that is not found had changed. This ensures that is consistent with the error assertion that we have in the code for the ruler. * Use the method from the interfaace to determine object storage * also include DeleteObject * appease the linter
chaudum
pushed a commit
that referenced
this pull request
Feb 10, 2022
We copied over the ruler code as part of #5089, and it seems that at the same time we stopped depending on the object storage implementation of Cortex. Turns out, there are (slight) differences between the clients which meant that response returned by the API when we try to get a rule group that is not found had changed. This ensures that is consistent with the error assertion that we have in the code for the ruler.
dannykopping
pushed a commit
that referenced
this pull request
Feb 10, 2022
* Ruler: Rule group not found API message We copied over the ruler code as part of #5089, and it seems that at the same time we stopped depending on the object storage implementation of Cortex. Turns out, there are (slight) differences between the clients which meant that response returned by the API when we try to get a rule group that is not found had changed. This ensures that is consistent with the error assertion that we have in the code for the ruler. * Use the method from the interfaace to determine object storage * also include DeleteObject * appease the linter
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 / why we need it:
As a part of removing Loki dependency on cortex packages, we decided to copy the
rulerpackage from cortex to Loki itself.Note that I decided to copy it into a
basesubpackage instead of merging with existingrulerpackage to make reviewing this easier. If you prefer that I try merging them into a single package at this same PR, just let me know.Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
basesubpackage instead of merging it with the existingrulerpackage to make reviewing this easier. If you prefer that I try merging them into a single package at this same PR, just let me know.Checklist
CHANGELOG.mdabout the changes.