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

Option to exclude default / hard-coded helm annotations for jobs and other relevant templates #17447

Closed
micke-a opened this issue Aug 5, 2021 · 4 comments
Labels
area:helm-chart Airflow Helm Chart kind:feature Feature Requests

Comments

@micke-a
Copy link

micke-a commented Aug 5, 2021

Description

Option to exclude default / hard-coded helm annotations for jobs and other relevant templates.

Use case / motivation

Ability to exclude default helm annotations will make it easier to use tools such as ArgoCD and its Helm integration to manage deployments.

ArgoCD has some understanding of Helm annotations ArgoCD docs but wants us to override certain things like "hook-weight" to make sure the resource is created at the right time.

Currently it is not possible to avoid the default annotations being created and we have tested to add-on the ArgoCD specific annotations but they don't override the default ones.
Which results in ArgoCD deployments not working due to e.g. deployments waiting forever for the db migration job to complete (which doesn't run).

Are you willing to submit a PR?

Possibly, new to Helm and unsure how I can actually test it out : )

Not been able to test this yet but started to work on a potential solution to this; adding defaultJobAnnotations: true configuration attribute.

Example:

config:
  migrateDatabaseJob:
    defaultJobAnnotations: false
    jobAnnotations:
      "argocd.argoproj.io/hook": PostSync
  
  createUserJob:
    defaultJobAnnotations: false
    jobAnnotations: 
      "argocd.argoproj.io/hook": PostSync

Which could be used like this in the template:


  annotations:
    {{- if .Values.migrateDatabaseJob.defaultJobAnnotations }}
    "helm.sh/hook": post-install,post-upgrade
    "helm.sh/hook-weight": "1"
    "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
    {{- end }}
    {{- if .Values.migrateDatabaseJob.jobAnnotations }}
    {{- toYaml .Values.migrateDatabaseJob.jobAnnotations | nindent 4 }}
    {{- end }}

@micke-a micke-a added the kind:feature Feature Requests label Aug 5, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 5, 2021

Thanks for opening your first issue here! Be sure to follow the issue template!

@angel9484
Copy link

Coming from #18172

Good idea!

I think that beyond to solve what technology is behind the behavior of the object scheduling order (argocd, vanilla helm...), the wait-for-migrations initContainer should be removed and separate the responsabilities of the migration job and the correct startup of the airflow parts. Is a solution more complex compared yours but I think is better in a long term

Kind regards!

@burakovsky
Copy link

burakovsky commented Dec 9, 2021

Is it possible to add a similar configuration for other resources with hardcoded helm annotations (extra-configmaps, redis-secrets.yaml) ? Are these annotations really required in configmaps and secrets as Helm creates it before most other resources?

@jedcunningham
Copy link
Member

This is supported now that #18776 has been merged.

@burakovsky, that is something I've been pondering as well. I'll try and take a look here soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart kind:feature Feature Requests
Projects
None yet
Development

No branches or pull requests

5 participants