-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: make helm hooks mandatory #102
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR @jbouchery! While the team reviews the PR, may I ask you to:
|
Hi @rhamzeh, Thanks for the feedback ! I Signed the Linux Foundation's EasyCLA as you said. I also reviewed the merge conflicts but my PR only involve a change of the Helm Chart, not the application. I don't think i have to bump de appVersion in this case. |
@@ -34,17 +34,6 @@ spec: | |||
serviceAccountName: {{ include "openfga.serviceAccountName" . }} | |||
securityContext: | |||
{{- toYaml .Values.podSecurityContext | nindent 8 }} | |||
{{- if and (has .Values.datastore.engine (list "postgres" "mysql")) .Values.datastore.applyMigrations }} |
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 understand the other changes in this PR, but I don't understand why you dropped this. Could you elaborate on why you removed this?
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.
pinging @jbouchery :)
Please don't male the hooks mandatory as it seams to be breaking on ArgoCD deployement... I had to do this :
in order to make it helm on argocd |
Description
I saw that this PR added the Helm Hooks to the migration job #70. I think it could be better to make them mandatory instead of default in the values (so it can't be overridden). In addition, i deleted the initContainer that is deprecated when using Helm Hooks.
References
The PR introducing the migration Helm Hooks #70
Review Checklist
main