Uh oh!
There was an error while loading. Please reload this page.
Replace Ansible with SQLAlchemy/PyMySQL for database creation. - #206
Replace Ansible with SQLAlchemy/PyMySQL for database creation.#206intlabs wants to merge 7 commits into
Conversation
alanmeadows
left a comment
There was a problem hiding this comment.
Good start Pete. A few comments below.
| @@ -0,0 +1,12 @@ | |||
| {{- define "helm-toolkit.secret_db_root" }} | |||
There was a problem hiding this comment.
This is not named after the file that contains it.
| @@ -0,0 +1,2 @@ | |||
| {{- $envAll := . }} | |||
| {{- include "helm-toolkit.secret_db_root" $envAll }} | |||
There was a problem hiding this comment.
Because this toolkit script leverages only values from .Values.database I would recommend that is what we pass. We have a mix between passing macros $envAll or . and the leafs that contain the data they need. We should standardize on one. I prefer leafs (.Values.database as its clear what input we are passing the macro but I'm flexible.
There was a problem hiding this comment.
That approach works for me.
There was a problem hiding this comment.
This work (and comments) have been made moot by: https://github.com/att-comdev/openstack-helm/pull/262/files#diff-728a997bda2af0665cbf1fde10e4231fR153, which is a better approach. For now I'll adopt this method verbatim, to allow work to continue on this PR.
| ip_address: "0.0.0.0" | ||
| database: | ||
| secret: |
There was a problem hiding this comment.
It's not clear to me how this differs from the root_password below
There was a problem hiding this comment.
This is the name of the secret, rather than the actual password. I'll try to make this clearer in a follow-up commit.
There was a problem hiding this comment.
That makes much more sense. I think just secret_name vs secret would of made me realize this.
There was a problem hiding this comment.
This will be refactored to match the style here: https://github.com/alanmeadows/aic-helm/blob/b40a7bf3be5bcc9960452b0ad72c6db9932f101e/keystone/values.yaml#L303. I'll insert the key k8s_secret_name as appropriate within this to define the name of the secret that will hold the derived SQLAlchemy connection string.
| secretKeyRef: | ||
| name: {{ .Values.database.secret.root }} | ||
| key: DB_CONNECTION | ||
| - name: OPENSTACK_CONFIG_FILE |
There was a problem hiding this comment.
Seems like this will keep growing, maybe our checker-helpers needs its own oslo config file which is just a config map? This may grow to unwieldy proportions. Understandably could be future work.
There was a problem hiding this comment.
The idea here is to consume the same config file as the service will consume, the job itself is what defines the scope here I think. Unless we are across purposes?
| mountPath: /tmp/db-create.py | ||
| subPath: db-create.py | ||
| readOnly: true | ||
| - name: pod-etc-service |
There was a problem hiding this comment.
Just a minor nit, not sure I fully grasp the naming conventions here... pod-etc-service really is an emptyDir, service-config is keystone.conf... I had to look down below two or three times - which either represents my own senility or the need to make this clearer
There was a problem hiding this comment.
I've used the naming conventions from harbor here:
- Prefixes:
- pod: represents a volume that is local to the pod, almost always an emptyDir
- service: a volume that is common the service, almost always a configMap, but may be a shared volume
- Suffixes:
- The mount location of the volume if only used once.
- If a configmap, just configmap (unless several are used, in which case I added a further suffix with some brief indicator of the contents)
Of course this comment contradicts reality, unless there is objection I'll update to make the above true.
To resume work on this PR we should merge the following:
As they make changes to the job layout that will cause conflicts with this work. |
intlabs
commented
Mar 16, 2017
Superseded by #289 |
What is the purpose of this pull request?: To replace ansible with PyMySQL for DB Creation
What issue does this pull request address?: This is part of the effort to become image agnostic.
Notes for reviewers to consider: This PR is intended to supersede #140
Specific reviewers for pull request: @alanmeadows, @srwilkers, @v1k0d3n