-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(helm): Expose all storage configuration #18509
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
Conversation
d6e699e to
8efde0b
Compare
| {{- if .Values.minio.enabled -}} | ||
| type: "s3" | ||
| s3: | ||
| bucketnames: ruler |
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.
| bucketnames: ruler | |
| bucketnames: {{ .Values.loki.storage.bucketNames.ruler }} |
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.
Same here, bucket names with internal minio deployment are hard-coded
loki/production/helm/loki/values.yaml
Lines 3532 to 3541 in ce03b79
| buckets: | |
| - name: chunks | |
| policy: none | |
| purge: false | |
| - name: ruler | |
| policy: none | |
| purge: false | |
| - name: admin | |
| policy: none | |
| purge: false |
| azure: | ||
| account_name: {{ .accountName }} | ||
| {{- toYaml (mergeOverwrite | ||
| dict |
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 not easily readable 🤔 Can we do it a bit differently? Maybe azure, gcs and s3 could be extracted into separate templates for better readibility?
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.
please check this out if it meets your expectations
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Quentin Bisson <[email protected]> Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jay Clifford <[email protected]>
|
Just kicking off the workflows for this one again. Thanks for your efforts here, both. Let me know when you are ready for it to be merged |
|
LGTM, nice work @jkroepke |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Jayclifford345
left a comment
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
Signed-off-by: J Stickler <[email protected]>
What this PR does / why we need it:
This PR exposes all storage properties similar to #17597. It also adds an backwards compatibility layer to preserve old camelCase values naming.
It should also cover some PR related to exposing newer storage properties.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR