-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 Tanka microservices: common environment variables #6124
Conversation
Signed-off-by: Ivan Rizzante <i.rizzante@gmail.com>
./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% |
Hello @kavirajk can you please take a look at this? |
@irizzant thanks for the PR. My primary concern here is empty Curious why not add Something similar to local loki = import 'lib/loki';
{
_config+: {
commonEnvs: [ envVar.new('AWS_CREDENTIALS')],
}
distributor_container+::
container.withEnvMixin(_config.commonEnvs)
}
Same for other pod(s) containers as well. In fact this is how we do internally to pass credentials to our pods. |
@kavirajk the empty array as value for The reason why I didn't use the approach you suggested is to avoid the override at container level.
without having to go through all the containers to add:
is a more friendly approach. |
./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% |
@irizzant I'm convinced. |
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 👍 Thanks @irizzant
What this PR does / why we need it:
This PR adds commonEnvs config parameter to specify variables common to all Pod and it was created to facilitate specifying S3 credentials.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md