-
Notifications
You must be signed in to change notification settings - Fork 207
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: support argocd app of apps deployment #300
Conversation
Thanks for this @starchx! We will definitely need some documentation updates which explain how this functionality can be used. |
@starchx echoing Kevin's comment: can you please attach documentation to the PR so that reviewers understand what the new functionality is? |
69f4293
to
88331a8
Compare
@kcoleman731 @shapirov103 Thanks for reviewing the PR. I have just added the doc with example code of how to enable During the development, I am aiming to have both CDK and Terraform consuming the same AddOns repo, which is why I also created this PR to address some consistent issues. Any feedback are much appreciated. |
@starchx thank you for making it mergeable. I was out last week, so slowly getting back. Will review this week. Thank you for the contribution! |
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.
@starchx fantastic PR. My apologies for taking a long time to review, it was related to the release and de-facto code freeze with the exception of bug fixes and doc updates.
* after the infrastructure and team provisioning is complete. | ||
* after the infrastructure and team provisioning is complete. | ||
* When GitOps mode is enabled via `ArgoGitOpsFactory` for deploying the AddOns, this bootstrap | ||
* repository will be used for provisioning all `HelmAddOn` based AddOns. | ||
*/ | ||
bootstrapRepo?: spi.ApplicationRepository; |
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 also was thinking about an approach for non helm add-ons (just k8s manifests that we could deploy with some templating/kustomize) that could also be gitops driven. This is for later though, but let me know if you saw any customers asking for it.
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.
Good callout. Will definitely keep eye on this. Going through the current blueprint addon list, these are the only two not using HelmAddOn: (exclude those EKS managed addons)
- SSM Addon
- XRay Addon
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.
Yeah, I hope it will be less and less. Such add-ons may be, however, custom. SSM and XRay standalone are less of a concern for me atm. XRay support is available through AppMesh add-on (enableTracing).
Thanks again for the PR review @shapirov103 All the suggestions are very good and making sense. I have updated the PR ready. There is only one comment on this one: #300 (comment) When you have time 🙂 |
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.
All looks great!
ArgoCD merging of values is reasonable. It can enable customers to pass additional values to the app of apps from add-on, so no disagreement.
Now we need to solve the approach for testing it. Ideally I would like to have three clusters in the e2e tests created: 1/ imperative add-ons through CDK (many customers will use just that), 2/ declarative GitOps with app of apps 3/ declarative GitOps with apps.
The above is ideal as I mentioned. To merge this, we can add pattern to the patterns repo for both approaches. This will however, require a version increment for this artifact.
So we have two options:
- e2e with 3 clusters
- a simpler approach with a pattern in the patterns repo.
Regardless, let's bump the version up to 1.1.0 - I am thinking a minor release.
* after the infrastructure and team provisioning is complete. | ||
* after the infrastructure and team provisioning is complete. | ||
* When GitOps mode is enabled via `ArgoGitOpsFactory` for deploying the AddOns, this bootstrap | ||
* repository will be used for provisioning all `HelmAddOn` based AddOns. | ||
*/ | ||
bootstrapRepo?: spi.ApplicationRepository; |
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.
Yeah, I hope it will be less and less. Such add-ons may be, however, custom. SSM and XRay standalone are less of a concern for me atm. XRay support is available through AppMesh add-on (enableTracing).
@shapirov103 , I am closing this PR in favor of #588 |
Issue #, if available:
#299
Description of changes:
This PR is based on @shapirov103 's branch and add App of Apps support on top.
A factory class is introduced to make it easy for our customers to enable or disable GitOps mode:
Tested by destroying the cluster and recreating the cluster. All enabled addons were installed correctly by ArgoCD via
App of Apps
:By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Shout out to @shapirov103 's guidance in getting this PR created.