-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Loki mixin: publish compiled version of the mixin #6254
Conversation
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
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.
This is super nice ❤️ Thanks @pracucci. Left few small nits. Overall looks great.
Makefile
Outdated
@@ -27,7 +27,7 @@ DOCKER_IMAGE_DIRS := $(patsubst %/Dockerfile,%,$(DOCKERFILES)) | |||
BUILD_IN_CONTAINER ?= true | |||
|
|||
# ensure you run `make drone` after changing this | |||
BUILD_IMAGE_VERSION := 0.20.4 | |||
BUILD_IMAGE_VERSION := publish-compiled-mixin-a795b93-WIP |
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.
@pracucci I don't if WIP suffix is intentional? I think WIP suffix comes from the fact, you may have some uncommitted changes while building image via make loki-build-image
.
We can get rid of it by removing all git modified files in local workspace.
Can we keep some static tag instead? (e.g: 0.20.5
)
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.
Yeah, see PR description:
Draft PR to get a feedback. Then I will open a dedicated PR to properly update the build image.
Given you're approving this PR, I'm now going to open a PR to publish 0.20.5 (following the 2 steps process in your instructions).
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.
See: #6255
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
Signed-off-by: Marco Pracucci <[email protected]>
Co-authored-by: Kaviraj Kanagaraj <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
91fc8ff
to
f2376d2
Compare
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
What this PR does / why we need it:
I'm leading an effort to publish and document dashboards, alerts and runbooks across Mimir, Loki and Tempo. In this PR I want to propose to publish a compiled version of the mixin (with default config), following the same pattern used by Mimir.
Documentation is out of the scope of this PR. Will be done in a follow up PR.
Which issue(s) this PR fixes:
Partially addresses #6234 (doc still missing)
Special notes for your reviewer:
N/A
Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md