-
Notifications
You must be signed in to change notification settings - Fork 16.6k
[stable/wordpress] Allow helm chart to receive an existing database or create it by itself Open, HighAll Users #773
[stable/wordpress] Allow helm chart to receive an existing database or create it by itself Open, HighAll Users #773
Conversation
|
Hi @jbianquetti-nami. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
1 similar comment
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
CLA signed. |
sameersbn
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.
Aside from my comments, i tested the changes and found them to be working. nice job!
stable/wordpress/values.yaml
Outdated
| ## Set to `yes` to allow the container to be started with blank passwords | ||
| ## ref: https://github.com/bitnami/bitnami-docker-wordpress#environment-variables | ||
| ## Defaults to no | ||
| ## allowEmptyPassword: |
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.
we generally add the default value to the values.yaml file. I'd suggest un-commenting changing this to
allowEmptyPassword: "no"
or if you think https://github.com/kubernetes/charts/pull/773/files#r105334999 makes sense, then you should be using
allowEmptyPassword: false
| imagePullPolicy: {{ default "" .Values.imagePullPolicy | quote }} | ||
| env: | ||
| - name: ALLOW_EMPTY_PASSWORD | ||
| value: {{ default "no" .Values.allowEmptyPassword | quote }} |
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.
if user specifies allowEmptyPassword value without qoutes, go will treat this as a boolean value. User will HAVE TO specify the allowEmptyPassword with double quotes in values.yaml, eg. allowEmptyPassword: "yes". I would not regard this as a stable UX. Instead i suggest using the following snippet.
{{- if .Values.allowEmptyPassword }}
- name: ALLOW_EMPTY_PASSWORD
value: "yes"
{{- end }} or
- name: ALLOW_EMPTY_PASSWORD
{{- if .Values.allowEmptyPassword }}
value: "yes"
{{- else }}
value: "no"
{{- end }} | ## allowEmptyPassword: | ||
|
|
||
|
|
||
|
|
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.
consider removing these white spaces
stable/wordpress/README.md
Outdated
| | `wordpressFirstName` | First name | `FirstName` | | ||
| | `wordpressLastName` | Last name | `LastName` | | ||
| | `wordpressBlogName` | Blog name | `User's Blog!` | | ||
| | `databaseName` | Database name to create | `bitnami_wordpress` | |
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.
The mariadb chart allows you to create a database and user credentials to access the database. Rather than configuring databaseName, databaseUser and databasePassword on the wordpres chart it think mariadbDatabase, mariadbUser and mariadbPassword should be configured on the mariadb chart, similar to how mariadbRootPassword is set.
|
@jbianquetti-nami please rebase |
|
Pulling this into Milestone 4 as it blocking other WordPress updates. @jbianquetti-nami you seem to have pulled in other commits on different charts, can you please fix that? |
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.
undo this change
…create it by itself
…create it by itself
cf6ad4c to
6383f70
Compare
sameersbn
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.
Apart from my comment about the default value of allowEmptyPassword, the changes look good to me.
stable/wordpress/README.md
Outdated
| | `wordpressFirstName` | First name | `FirstName` | | ||
| | `wordpressLastName` | Last name | `LastName` | | ||
| | `wordpressBlogName` | Blog name | `User's Blog!` | | ||
| | `allowEmptyPassword` | Allow DB blank passwords | `no` | |
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.
The default value is yes
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.
Lost while rebasing. I will update it in a few minutes.
|
Thanks @jbianquetti-nami! |
…r create it by itself Open, HighAll Users (helm#773) * Allow to the Wordpress helm chart to receive an existing database or create it by itself * Allow to the Wordpress helm chart to receive an existing database or create it by itself * added links to documentation for new env vars * added changes with compatibility threats for 0.4.x chart series. Bumping to 0.5.0 * added defaults for many values and fixes typos * added defaults for many values and fixes typos * fix typo * cosmetic revision * updated dependency on mariadb chart 0.5.10 * environment vars clean up * Readme reformatting * added changes with compatibility threats for 0.4.x chart series. Bumping to 0.5.0 * added defaults for many values and fixes typos * fix typo * Allow to the Wordpress helm chart to receive an existing database or create it by itself * added defaults for many values and fixes typos * fix typo * fixing default value in README for AllowEmptyPassword
|
Would it be possible to get an example of how I would use helm to install wordpress while specifying a separate mysql host, not creating one on the cluster? I'm not seeing a way to skip installation of mariadb and specify a remote host, which I think is what this PR was intended for based on the title |
Summary of changes:
Now, we can also create the database from the application side.
The ENV vars are:
Also, we have added an additional security measure regarding passwordless logins.
"allowEmptyPassword": {"default": "no", "type": "choice", "validValues": ["yes", "no"], "description": "Allow to set an empty password"}In case the container does not have the related ENV var enabled. The ENV var is:
ALLOW_EMPTY_PASSWORD