-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add support for argocd and custom annotations to the init job #246
Conversation
Added additional annotations configuration fo the init job.
Can this PR get some love soon? ❤️ I'm also affected by this issue. 😢 |
@pseudomuto maybe you can review 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.
Overall this seems reasonable to me 👍
I think we should add/update a test here just to ensure this is exercised and not lost in a future update.
@@ -21,6 +21,10 @@ metadata: | |||
annotations: | |||
helm.sh/hook: post-install,post-upgrade | |||
helm.sh/hook-delete-policy: before-hook-creation | |||
argocd.argoproj.io/hook: Sync |
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.
Rather than hardcoding this, could we instead just have anyone that would like to include this define it in jobAnnotations
?
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 thought the same, @pseudomuto.
@pseudomuto, what kind of test exactly would you like to be implemented? |
@codelite7, please, let me know if you would like me to contribute. Considering the Github profile, I'd assume that @pseudomuto is based in Canada. But I don't know where you're based in. So, as I'm in Europe (CEST) there might be some time zone difference that would delay things a bit, unfortunately. |
We have some template tests in https://github.com/cockroachdb/helm-charts/blob/9f0f8497b729885f0970c8351bdac2682f4ef191/tests/template/cockroachdb_helm_template_test.go. I was thinking we could just add one there to ensure jobAnnotations is properly set on init job and nothing else. |
In some cases, there is a need for adding custom annotations to the Init job. For example, when using ArgoCD, we need to instruct ArgoCD to start the job still during the Sync phase and not as a PostSync hook. Otherwise, the job never starts. And this is done by adding the following annotation to the job. ``` argocd.argoproj.io/hook: Sync ``` This is to continue the work started in cockroachdb#246 to address cockroachdb#149
@pseudomuto, @rail, and @codelite7, I've ported the changes from this PR to #250 to include the changes suggested throughout this thread. Please, give it some love. ❤️ |
Closing in favour of #250 |
In some cases, there is a need for adding custom annotations to the Init job. For example, when using ArgoCD, we need to instruct ArgoCD to start the job still during the Sync phase and not as a PostSync hook. Otherwise, the job never starts. And this is done by adding the following annotation to the job. ``` argocd.argoproj.io/hook: Sync ``` This is to continue the work started in cockroachdb#246 to address cockroachdb#149
In some cases, there is a need for adding custom annotations to the Init job. For example, when using ArgoCD, we need to instruct ArgoCD to start the job still during the Sync phase and not as a PostSync hook. Otherwise, the job never starts. And this is done by adding the following annotation to the job. ``` argocd.argoproj.io/hook: Sync ``` This is to continue the work started in #246 to address #149
Added argocd sync hook to the init job annotations.
Added additional annotations configuration fo the init job.
This addresses #149