-
Notifications
You must be signed in to change notification settings - Fork 220
Add option to set MongoDB URL string in a secret #568
base: master
Are you sure you want to change the base?
Conversation
Simplify the conditional logic with "mongodb.url" helper definition. Also remove HTTP_PROXY AND HTTPS_PROXY ENV vars outside the mongodb.enabled conditional wrapper - I believe that was an unintentional result of two PRs merging cleanly in close proximity: - helm#547 - helm#554 Signed-off-by: Scott Rigby <[email protected]>
Signed-off-by: Scott Rigby <[email protected]>
…implify Signed-off-by: Scott Rigby <[email protected]>
{{- else if $global.Values.global.mongoUrl }} | ||
- --mongo-url={{ $global.Values.global.mongoUrl }} | ||
{{- else if $global.Values.global.mongoUrlSecret }} | ||
- --mongo-url={{ "$MONGO_URL" }} |
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.
Are the {{ }}
needed here? I think this could just be
- --mongo-url={{ "$MONGO_URL" }} | |
- --mongo-url="$MONGO_URL" |
- name: MONGO_PASSWORD | ||
valueFrom: | ||
secretKeyRef: | ||
key: mongodb-root-password | ||
name: {{ template "mongodb.fullname" $global }} | ||
{{- end }} | ||
{{- if $global.Values.global.mongoUrlSecret }} |
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 could be elseif I guess, though does not really matter much...
# You may set the connection URL in one of two ways: | ||
# - mongoUrl: store the connection string directly in Helm values | ||
# - mongoUrlSecret: name of the secret where the connection string is stored, | ||
# where the key is "mongo-url-secret". See NOTES.txt. | ||
global: |
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.
What do you think about moving this configuration to externalDatabase
? e.g. https://github.com/helm/charts/blob/master/stable/wordpress/values.yaml#L78. Then in the future we are one step closer to splitting up the options in host, user, password, etc. It would make this a breaking change, but I doubt anyone is using this setting yet so the risk should be very low.
On one hand this could be further simplified by removing the string option, and only allowing the secret option. But that would require a MAJOR version bump (removing an existing option), whereas this only requires MINOR. @prydonius What do you think?