-
Notifications
You must be signed in to change notification settings - Fork 44
feat(initcontainers): add api volume mounts to init containers to sup… #392
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
matthewelwell
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.
Thanks @scarlson for submitting this - it makes sense to add these attributes. I've added a minor question, and then we can get this merged.
emyller
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.
Thanks for the contribution! I think it's a good idea overall to share volumes with essential containers.
I have only a few comments, can you please check?
| imagePullPolicy: {{ .Values.api.image.imagePullPolicy | default .Values.global.image.imagePullPolicy }} | ||
| args: ["migrate"] | ||
| env: {{ include (print $.Template.BasePath "/_api_environment.yaml") . | nindent 8 }} | ||
| env: {{ include (print $.Template.BasePath "/_api_environment.yaml") . | nindent 8 -}} |
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'm curious — did you find any issues that required chomping?
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.
…port SSL auth methods that require certs to be mounted
ca856c3 to
e92dd0e
Compare
emyller
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! Thanks again for the contribution @scarlson.
matthewelwell
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.
Approving this but we'll hold off merging until the CI issues are resolved as per the comment here.

…port SSL auth schemas that require certs to be mounted
Thanks for submitting a PR! Please check the boxes below:
Changes
Add api volume mounts to
migratedbandbootstrapinit containers to allow for SSL auth schemas in those containers.How did you test this code?
Set volume mount values for properly mounting certs.
Rendered manifests with the changes and applied to test k8s cluster.