Skip to content
This repository was archived by the owner on Jul 26, 2022. It is now read-only.

feat(helm): add in ability to inject init containers in to deployment from values #615

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

chrismellard
Copy link
Contributor

No description provided.

@chrismellard chrismellard force-pushed the master branch 3 times, most recently from fac4b23 to aabe0a6 Compare February 3, 2021 07:54

deploymentInitContainers: {}

emptyDirVolumes: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do more generic and add extraVolumeMounts and extraVolumes instead ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR to reflect your suggestion. Thanks for the review 👍

@Flydiverny Flydiverny changed the title feat: add in ability to inject init containers in to deployment from values feat(helm): add in ability to inject init containers in to deployment from values Feb 5, 2021
rawlingsj added a commit to rawlingsj/jx-gitops that referenced this pull request Feb 8, 2021
Copy link
Member

@Flydiverny Flydiverny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we can keep it even more "standard". At least I think this pattern is pretty common in charts. I could be wrong a bit out of the loop 😄

Comment on lines 68 to 70
{{- with .Values.extraVolumeMounts }}
{{- range $key, $value := . }}
- name: {{ $key }}
mountPath: {{ $value.mountPath }}
readOnly: {{ $value.readOnly }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we could simplify this

Suggested change
{{- with .Values.extraVolumeMounts }}
{{- range $key, $value := . }}
- name: {{ $key }}
mountPath: {{ $value.mountPath }}
readOnly: {{ $value.readOnly }}
{{- end }}
{{- end }}
{{- if .Values.extraVolumeMounts }}
{{ toYaml .Values.extraVolumes | nindent 12 }}
{{- end }}

Comment on lines 108 to 106
{{- with .Values.extraVolumes }}
{{- range $key, $value := . }}
- name: {{ $key }}
{{- toYaml $value | nindent 8 }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we could simplify this

Suggested change
{{- with .Values.extraVolumes }}
{{- range $key, $value := . }}
- name: {{ $key }}
{{- toYaml $value | nindent 8 }}
{{- end }}
{{- end }}
{{- if .Values.extraVolumes }}
{{ toYaml .Values.extraVolumes | nindent 8 }}
{{- end }}

Comment on lines 133 to 140
#extraVolumes:
# namedVolume:
# emptyDir: {}
#
#extraVolumeMounts:
# namedVolume:
# mountPath: /usr/path
# readOnly: false
Copy link
Member

@Flydiverny Flydiverny Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#extraVolumes:
# namedVolume:
# emptyDir: {}
#
#extraVolumeMounts:
# namedVolume:
# mountPath: /usr/path
# readOnly: false
extraVolumes: []
# - name: namedVolume
# emptyDir: {}
#
extraVolumeMounts: []
# - name: namedVolume
# mountPath: /usr/path
# readOnly: false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good - more likely me out of the loop ;) Updated.

Copy link
Member

@Flydiverny Flydiverny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 merging assuming e2e tests doesnt magically fail

@chrismellard
Copy link
Contributor Author

Hold off. spotted one problem

…values

chore: added in jetbrains ignore files


x
@chrismellard
Copy link
Contributor Author

Resolved. Was outputting volumes yaml in volumeMounts block :)

@Flydiverny Flydiverny merged commit 21acce1 into external-secrets:master Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants