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(argo-cd): Update to use v2.3.1 release #1162

Merged
merged 17 commits into from
Mar 16, 2022

Conversation

mikeeq
Copy link
Contributor

@mikeeq mikeeq commented Mar 7, 2022

That's my first PR to your repository, I will try to follow the checklist and fix all of the issues.

I'm not sure if the chart version should be bumped in minor or major version.

Resolves #1115


Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

Changes are automatically published when merged to master. They are not published on branches.

@github-actions github-actions bot added the size/M label Mar 7, 2022
@mikeeq mikeeq force-pushed the feature/argocd-2-3-0 branch 3 times, most recently from 98cbc09 to 5391cfc Compare March 7, 2022 12:58
@mikeeq mikeeq changed the title Feature: Update argo-cd chart to use v2.3.0 feat: update argo-cd chart to use v2.3.0 release Mar 7, 2022
Copy link
Contributor

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

lgtm

@steinarox
Copy link

Is there any config values for notifications or appsets that needs to be added?
like the argocd-notifications-cm https://argo-cd.readthedocs.io/en/latest/operator-manual/notifications/templates/

and appset crd

@davidkarlsen
Copy link
Contributor

Is there any config values for notifications or appsets that needs to be added? like the argocd-notifications-cm argo-cd.readthedocs.io/en/latest/operator-manual/notifications/templates

and appset crd

Awaiting response to this before we let it run and merge.

@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 7, 2022

Is there any config values for notifications or appsets that needs to be added? like the argocd-notifications-cm argo-cd.readthedocs.io/en/latest/operator-manual/notifications/templates
and appset crd

Awaiting response to this before we let it run and merge.

Who should respond to that? I'm happy to add everything that's required here, just let me know what exactly ;)

@steinarox
Copy link

seems like the appset crd is not in the argoproj manifests. but the kustomize file points here https://raw.githubusercontent.com/argoproj/applicationset/master/manifests/install.yaml

@github-actions github-actions bot added size/XXL and removed size/M labels Mar 7, 2022
@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 7, 2022

ApplicationSet CRD was added ;)

EDIT: I removed status section

...
status:
  acceptedNames:
    kind: ""
    plural: ""
  conditions: []
  storedVersions: []

from appset crd as that section is also removed for extension crd.

@mikeeq mikeeq force-pushed the feature/argocd-2-3-0 branch from e50e9aa to e3a3bb9 Compare March 7, 2022 13:44
Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

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

Hmm we need to implement multiple things as we are discussing here: #1115

@mkilchhofer mkilchhofer changed the title feat: update argo-cd chart to use v2.3.0 release feat(argo-cd): Update to use v2.3.0 release Mar 7, 2022
@mbevc1 mbevc1 added the argo-cd label Mar 7, 2022
@pashcan
Copy link

pashcan commented Mar 7, 2022

Is there any config values for notifications or appsets that needs to be added? like the argocd-notifications-cm argo-cd.readthedocs.io/en/latest/operator-manual/notifications/templates
Who should respond to that? I'm happy to add everything that's required here, just let me know what exactly ;)

The upgrade to v2.3.0 needs to include functionality of that version. Controller deployments, serviceaccounts, roles and rolebindings, default values, etc. Pretty much most of these
https://github.com/argoproj/argo-helm/tree/master/charts/argocd-applicationset/templates
https://github.com/argoproj/argo-helm/tree/master/charts/argocd-notifications/templates

Your commit only includes CRDs but this is a great start!

@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 7, 2022

The question is how we want to include those argocd-applicationset and argocd-notifications charts? Add them as dependencies in Chart.yaml or migrate those yaml files over to argo-cd chart?

@mbevc1
Copy link
Collaborator

mbevc1 commented Mar 7, 2022

Not sure we had a consensus, but more context is in #1115

@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 10, 2022

Guys, were there any official decisions made on how we want to push that forward?

@pashcan
Copy link

pashcan commented Mar 10, 2022

Seems like consensus is to include applicationsets & notifications charts into the ArgoCD one and cut a new major version.
#1115 (comment)

@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 11, 2022

I pushed the latest changes and accordingly to the previous post I migrated applicationset and notification chart templates to the main argo-cd chart. It's an open PR for any comments, just wanted to know if that was a goal and I'm looking forward to any suggestions ;)

I will squash commits when all be done and polished ;)

EDIT. @jasonmacdonald in the linked issue is mentioning that there were some changes in applicationset CRDs, but I couldn't find any (maybe they are not merged yet 🤷).

EDIT2. There are still some changes required in _helpers.tpl and I'm currently working on them.

@mikeeq mikeeq changed the title feat(argo-cd): Update to use v2.3.0 release feat(argo-cd): Update to use v2.3.1 release Mar 11, 2022
@steinarox
Copy link

steinarox commented Mar 11, 2022

@mikeeq i am not sure about deleting the old charts in this PR. This can confuse existing users when the chart and files disappear. The best path imo would be to deprecate the charts and write in their readmes that this is the new chart in a separate PR

@mikeeq
Copy link
Contributor Author

mikeeq commented Mar 11, 2022

@steinarox no problem, I will revert them ;)

EDIT. It would be very useful if someone could test applicationset and notifications changes, I deployed it with a default values and they seem to be up and running but I haven't used them before, so maybe someone already has a working configuration and can test it - thanks in advance ;)

@mikeeq mikeeq requested a review from mkilchhofer March 14, 2022 14:37
Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>
@mikeeq mikeeq force-pushed the feature/argocd-2-3-0 branch from 1231ea7 to 747d10d Compare March 15, 2022 14:53
mkilchhofer and others added 7 commits March 15, 2022 17:08
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>
Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>
Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>
Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
…epoServer"

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
…ller

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
charts/argo-cd/README.md Outdated Show resolved Hide resolved
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
@mkilchhofer mkilchhofer merged commit 32a0605 into argoproj:master Mar 16, 2022
terrych0u pushed a commit to terrych0u/argo-helm that referenced this pull request Aug 4, 2022
* feat(argo-cd): Update to use v2.3.1 release

Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>

* Move applicationSet.* and notifications.* to a dedicated location

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Fix typo in deployment.yaml

Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>

* Fix notifications bot deployment

Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>

* Update README.md

Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>

* Update README.md and fix slack bot image

Signed-off-by: mikeeq <miotk.mikolaj@gmail.com>

* Drop nameOverride for new components as we use the global one

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Drop "applicationSet.args.namespace" and "applicationSet.args.argocdRepoServer"

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Consistent use of volumes (gpg, tls, knownHosts, extra)

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Bump major chart version and place a note in teh Upgrading section

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Change "enableStatefulSet: true" as we are in a major release now

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Drop PSP of applicationSet

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Fix applicationset webhook-ingress

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Drop unused variables in values.yaml and README

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Also set namespace and argocd-repo-server args on notification-controller

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Consistent use of "Argo CD" instead of "ArgoCD"

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

* Trigger CI

Signed-off-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>

Co-authored-by: Marco Kilchhofer <mkilchhofer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

argo-cd (v2.3.x): Integrate ApplicationSets / Notifications in ArgoCD helm chart
7 participants