-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
fix(helm): Fix GEL image tag, bucket name and proxy URLs #12878
Conversation
Set the correct enterprise-logs image tag to 3.0.1 (without the 'v') Use the correct proxy URLs on the gateway when deploying in SSD mode Use the user's configured admin bucket name Clarify README to note support for distributed mode
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.
Looks great, thanks for catching these. Can you add a default for the admin bucket only in the case of using minio, and make sure it's empty otherwise?
production/helm/loki/values.yaml
Outdated
@@ -480,7 +480,7 @@ enterprise: | |||
admin_client: | |||
storage: | |||
s3: | |||
bucket_name: admin | |||
bucket_name: {{ .Values.loki.storage.bucketNames.admin }} |
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.
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.
I've updated this now. I've separated the admin storage config out into its own separate Helm value (enterprise.adminClientConfig
), which includes a default for Minio only.
To set any other storage options (e.g. AWS, GCS, etc), users can just override that adminClientConfig
value.
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.
Ideally a user only has to provide loki.storage.bucketNames
and everything else should just work. I think we should default the admin config to use loki.storage.bucketNames.admin
in all cases but minio, and in the minio case we can hardcode it. I don't think we want to force a user to have to provide a whole adminClient
section when they are not using minio.
Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
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.
looks great, thanks for addressing all my feedback!
Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]>
Signed-off-by: Vladyslav Diachenko <[email protected]> Co-authored-by: Vladyslav Diachenko <[email protected]>
What this PR does / why we need it:
The chart doesn't currently seem to deploy GEL without having to do a couple of workarounds. This PR:
3.0.1
(without thev
prefix).Which issue(s) this PR fixes:
Fixes #12845
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs 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.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR