openfga / helm-charts

Official Helm charts for the OpenFGA project.
https://openfga.dev
Apache License 2.0
18 stars 29 forks source link

feat: make helm hooks mandatory #102

Open jbouchery opened 5 months ago

jbouchery commented 5 months ago

Description

I saw that this PR added the Helm Hooks to the migration job https://github.com/openfga/helm-charts/pull/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 https://github.com/openfga/helm-charts/pull/70

Review Checklist

linux-foundation-easycla[bot] commented 5 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

rhamzeh commented 5 months ago

Thanks for the PR @jbouchery!

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

jbouchery commented 5 months ago

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.

supercairos commented 1 month ago

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