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

Net 5229 create dedicated argocd stanza #2785

Merged
merged 11 commits into from
Aug 31, 2023

Conversation

absolutelightning
Copy link
Contributor

@absolutelightning absolutelightning commented Aug 17, 2023

Changes proposed in this PR:

  • Add support for global.argocd.enabled flag in values.yaml which adds the
        "argocd.argoproj.io/hook": "Sync"
        "argocd.argoproj.io/hook-delete-policy": "HookSucceeded"

in the jobs -
server-acl-init-job

How I've tested this PR:

  • Bats Tests
    Manual Tests -
  • Created github pages to host helm charts following this medium article
  • Forked the repo and created a new branch - https://github.com/absolutelightning/consul-minikubes/tree/test-argocd.
  • Updated the repo with values from github helm charts created in step 1
  • Ran bash argocd.sh
  • kubectl port-forward svc/argocd-server -n argocd 8087:443
  • argocd admin initial-password -n argocd
  • Input username - admin and password output from previous step in UI at localhost:8087.
  • Recorded video
demoargocd.mov

Checklist:

@absolutelightning absolutelightning added the pr/no-backport signals that a PR will not contain a backport label label Aug 17, 2023
@absolutelightning absolutelightning marked this pull request as ready for review August 17, 2023 06:52
@absolutelightning absolutelightning added the consul-india PRs/Issues assigned to Consul India team label Aug 17, 2023
Copy link
Contributor

@Ganeshrockz Ganeshrockz left a comment

Choose a reason for hiding this comment

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

LGTM

charts/consul/templates/gateway-cleanup-job.yaml Outdated Show resolved Hide resolved
@lkysow
Copy link
Member

lkysow commented Aug 17, 2023

I don't think you need those annotations on all the jobs, just the jobs that don't have helm hook annotations already: https://argo-cd.readthedocs.io/en/stable/user-guide/helm/#helm-hooks

Argo CD supports many (most?) Helm hooks by mapping the Helm annotations onto Argo CD's own hook annotations:

Also how can we ensure Jobs in the future get this hook added? Can you write a test that ensures Job's that don't have helm hook annotations also have this argocd stanza.

@absolutelightning
Copy link
Contributor Author

absolutelightning commented Aug 18, 2023

Hey @lkysow, I have trouble understanding your comment.

  1. I checked this link I need argocd.argoproj.io/hook: Sync The documentation doesn't have - it only has PreSync.
  2. How can I write test for job which will come in future. Is there a way?

@lkysow
Copy link
Member

lkysow commented Aug 21, 2023

Have you tried this out? I don't think it will work as written. For example, the gateway cleanup job is marked as a pre-delete job but this change maps it to an argocd Sync hook. So previously the hook would only run right before the user deletes consul but now it's running every single time they do a deploy of the helm chart.

The gateway-resources job is marked at post-install,post-upgrade which based on the docs is automatically mapped to an argocd PostSync but your change makes it Sync. Same for server-acl-init-cleanup.

I think you don't need to touch the gateway resources or server-acl-init-cleaup jobs but would be best to test that and verify. For the gateway-cleanup job I'm not exactly sure what that does so will need to investigate.

In terms of testing, I was thinking a test similar to

#--------------------------------------------------------------------
# template consul.fullname
#
# This test ensures that we use {{ template "consul.fullname" }} everywhere instead of
# {{ .Release.Name }} because that's required in order to support the name
# override settings fullnameOverride and global.name. In some cases, we need to
# use .Release.Name. In those cases, add your exception to this list.
#
# If this test fails, you're likely using {{ .Release.Name }} where you should
# be using {{ template "consul.fullname" }}
@test "helper/consul.fullname: used everywhere" {
cd `chart_dir`
# Grep for uses of .Release.Name that aren't using it as a label.
local actual=$(grep -r '{{ .Release.Name }}' templates/*.yaml | grep -v -E 'release: |-release-name=' | tee /dev/stderr )
[ "${actual}" = '' ]
}
#--------------------------------------------------------------------
# template namespace
#
# This test ensures that we set "namespace: " in every file. The exceptions are files with CRDs and clusterroles and
# clusterrolebindings.
#
# If this test fails, you're likely missing setting the namespace.
@test "helper/namespace: used everywhere" {
cd `chart_dir`
# Grep for files that don't have 'namespace: ' in them
local actual=$(grep -L 'namespace: ' templates/*.yaml | grep -v 'crd' | grep -v 'clusterrole' | grep -v 'gateway-gateway' | tee /dev/stderr )
[ "${actual}" = '' ]
}
#--------------------------------------------------------------------
# component label
#
# This test ensures that we set a "component: <blah>" in every file.
#
# If this test fails, you're likely missing setting that label somewhere.
@test "helper/component-label: used everywhere" {
cd `chart_dir`
# Grep for files that don't have 'component: ' in them
local actual=$(grep -L 'component: ' templates/*.yaml | tee /dev/stderr )
[ "${actual}" = '' ]
}
which checks that all files that have a Job either have helm hook annotations or the argocd templating. So that if someone adds a Job in the future without either of those annotations that the test will fail to remind them to add one of the annotations.

@curtbushko
Copy link
Contributor

I think this needs to be tested with a Kind cluster and ArgoCD, there are too many unknowns.

Here is a script that you can use as a starting point for setting everything up...

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Great work!

.changelog/2785.txt Outdated Show resolved Hide resolved
@david-yu david-yu added the backport/1.2.x This release branch is no longer active. label Aug 28, 2023
@david-yu david-yu added backport/1.0.x backport/1.1.x Backport to release/1.1.x branch labels Aug 28, 2023
@david-yu
Copy link
Contributor

Added backports to the PR.

@david-yu david-yu removed the pr/no-backport signals that a PR will not contain a backport label label Aug 28, 2023
@absolutelightning absolutelightning enabled auto-merge (squash) August 29, 2023 02:36
@curtbushko curtbushko closed this Aug 31, 2023
@curtbushko curtbushko reopened this Aug 31, 2023
@absolutelightning absolutelightning merged commit ef30dc0 into main Aug 31, 2023
25 of 35 checks passed
@absolutelightning absolutelightning deleted the NET-5229-Create-dedicated-argocd-stanza branch August 31, 2023 02:40
@david-yu
Copy link
Contributor

@absolutelightning Could you make sure this gets backported? Thank you!

@absolutelightning
Copy link
Contributor Author

sure @david-yu will do it today. Thanks.

absolutelightning added a commit that referenced this pull request Aug 31, 2023
* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml

Co-authored-by: Ganesh S <[email protected]>

* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt

Co-authored-by: Luke Kysow <[email protected]>

---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
absolutelightning added a commit that referenced this pull request Aug 31, 2023
* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml

Co-authored-by: Ganesh S <[email protected]>

* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt

Co-authored-by: Luke Kysow <[email protected]>

---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
absolutelightning added a commit that referenced this pull request Aug 31, 2023
* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml

Co-authored-by: Ganesh S <[email protected]>

* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt

Co-authored-by: Luke Kysow <[email protected]>

---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
absolutelightning added a commit that referenced this pull request Aug 31, 2023
…e 1.2.x (#2872)

Net 5229 create dedicated argocd stanza (#2785)

* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml



* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt



---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
absolutelightning added a commit that referenced this pull request Aug 31, 2023
… 1.1.x (#2873)

Net 5229 create dedicated argocd stanza (#2785)

* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml



* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt



---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
absolutelightning added a commit that referenced this pull request Aug 31, 2023
… 1.0.x (#2874)

Net 5229 create dedicated argocd stanza (#2785)

* enable argocd

* adds bats test and setting argo annotations if global.argocd.enabled = true

* update comment

* added change log

* Update charts/consul/templates/gateway-cleanup-job.yaml



* comments fixes

* fix line diff

* change log fix

* fix comment

* Update .changelog/2785.txt



---------

Co-authored-by: Ganesh S <[email protected]>
Co-authored-by: Luke Kysow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1.x Backport to release/1.1.x branch backport/1.2.x This release branch is no longer active. consul-india PRs/Issues assigned to Consul India team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants