Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbouchery
Copy link

@jbouchery jbouchery commented Jan 23, 2024

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

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@jbouchery jbouchery requested a review from a team as a code owner January 23, 2024 16:47
Copy link

linux-foundation-easycla bot commented Jan 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@rhamzeh
Copy link
Member

rhamzeh commented Jan 29, 2024

Thanks for the PR @jbouchery!

While the team reviews the PR, may I ask you to:

  • Sign the Linux Foundation's EasyCLA? As a Cloud Native Computing Sandbox project, we cannot merge PRs that haven't signed the CLA.
  • Resolve the merge conflicts - There seem to be some merge conflicts around your PR (Mostly the app version. You may have to bump this again if another PR is merged before yours)

@jbouchery
Copy link
Author

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 }}
Copy link
Collaborator

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?

Copy link
Member

Choose a reason for hiding this comment

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

pinging @jbouchery :)

@supercairos
Copy link

supercairos commented Jun 3, 2024

Please don't male the hooks mandatory as it seams to be breaking on ArgoCD deployement...

I had to do this :

  migrate:
    annotations:
      helm.sh/hook: null
      helm.sh/hook-weight: null
      helm.sh/hook-delete-policy: null 

in order to make it helm on argocd

@rhamzeh rhamzeh marked this pull request as draft August 7, 2024 14:07
@rhamzeh rhamzeh added the hooks label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants