argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

feat(cli): add cmd to preview generated apps of appsets (#10895)

Open agaudreault opened this issue 1 year ago • 8 comments

  • depends on https://github.com/argoproj/argo-cd/pull/15741 having the list of application (must be merged and this code updated to use the resource field)

Closes #10895

The goal of this PR is to update the argocd appset create command to add a --dry-run argument. When it is specified, the server will call the applicationSet templating and app generation logic to evaluate and transform all the generator. It will use the resulting applications to generate a list of "generated" application.

./dist/argocd appset create --dry-run ./appset.yaml -o json | jq -r '.status.applicationStatus[].application'
ApplicationSet 'tests.simple-application' unchanged (dry-run)
tests.simple-application.dev
tests.simple-application.production-1
tests.simple-application.production-2
tests.simple-application.staging

The output of this command will allow to compare the list of resulting applications with the output of argocd appset get tests.simple-application -o json | jq -r '.status.applicationStatus[].application to know which app will be added or removed once the manifest is applied.

Suggestions

I think the CLI command could be argocd appset template ./appset.yaml -o json and return the list of templated applications with the full object and not only their name. Some people expressed the need to have the whole applications so they could validate the logic client-side.

With the current implementation, only the app name is returned in create and the get.

An implementation to be able to do client-side diff would be to have the template command return the current AppSet with all the applications templated, and modify the get to return the full templated application as well (this would avoid an argocd app get for each generated apps).

Checklist:

  • [x] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • [x] The title of the PR states what changed and the related issues number (used for the release note).
  • [x] The title of the PR conforms to the Toolchain Guide
  • [x] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • [x] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • [x] Does this PR require documentation updates?
  • [x] I've updated documentation as required by this PR.
  • [x] I have signed off all my commits as required by DCO
  • [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • [x] My build is green (troubleshooting builds).
  • [x] My new feature complies with the feature status guidelines.
  • [x] I have added a brief description of why this PR is necessary and/or what this PR solves.
  • [x] Optional. My organization is added to USERS.md.
  • [x] Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

agaudreault avatar Jan 08 '24 17:01 agaudreault

Codecov Report

Attention: Patch coverage is 62.45955% with 116 lines in your changes missing coverage. Please review.

Project coverage is 50.24%. Comparing base (4c6ad9d) to head (19b0871). Report is 11 commits behind head on master.

:exclamation: Current head 19b0871 differs from pull request most recent head 02bb52a

Please upload reports for the commit 02bb52a to get more accurate results.

Files Patch % Lines
cmd/argocd-server/commands/argocd_server.go 0.00% 46 Missing :warning:
cmd/argocd/commands/applicationset.go 0.00% 27 Missing :warning:
cmd/argocd/commands/headless/headless.go 0.00% 22 Missing :warning:
applicationset/controllers/template/template.go 83.01% 7 Missing and 2 partials :warning:
server/applicationset/applicationset.go 82.22% 4 Missing and 4 partials :warning:
applicationset/generators/scm_provider.go 70.00% 3 Missing :warning:
applicationset/generators/pull_request.go 75.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16781      +/-   ##
==========================================
- Coverage   50.28%   50.24%   -0.05%     
==========================================
  Files         312      315       +3     
  Lines       43020    43120     +100     
==========================================
+ Hits        21633    21665      +32     
- Misses      18905    18971      +66     
- Partials     2482     2484       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 10 '24 14:01 codecov[bot]

This feature is really needed in order to do any meaningful dryrun if using applicationset.

In production environment, we have to review the dryrun changes before any updates to applicationset could be landed. With this PR in place, we could get a list of applications, and then we could iterate the list of applications to get its diff via "argocd app diff" command at least.

Without this PR in place, there's no way to do any meaningful dryrun for applicationset.

This is the most needed feature for argocd applicationset.

haiwu avatar Jan 22 '24 18:01 haiwu

@argoproj/argocd-approvers ~~what do you think if we merge this as an "alpha" feature, and when the dependency is merged, we can change the output of the command to use the new fields? Maybe with a warning in the CLI doc?~~ Never mind, I forgot that the current get command does not return the list of applications when progressive sync is not present. So it does need the dependency.

The dependency seems to be coupled with a few other PRs and may take some time to merge. @alexymantha @csantanapr @reggie-k

agaudreault avatar Jan 22 '24 18:01 agaudreault

👍 Long-waited feature to allow simple and reliable validations on any non-trivial manifests repo that uses appset

cheskayang avatar Jan 31 '24 22:01 cheskayang

@agaudreault

The dependency seems to be coupled with a few other PRs and may take some time to merge. @alexymantha @csantanapr @reggie-k

Is this related to the sharding algorithms based on apps? Could you list of PRs that is holding this back?

csantanapr avatar Mar 08 '24 15:03 csantanapr

Initially, this PR was treated as a dependency for https://github.com/argoproj/argo-cd/pull/15731/files/19c5c0812ce859e97fb85ef1e9e63a2d38d32bb1 But lately I got an update from @crenshaw-dev that it was de-coupled and treated as an independent PR.

reggie-k avatar Mar 09 '24 06:03 reggie-k

@agaudreault

The dependency seems to be coupled with a few other PRs and may take some time to merge. @alexymantha @csantanapr @reggie-k

Is this related to the sharding algorithms based on apps? Could you list of PRs that is holding this back?

It only depends on https://github.com/argoproj/argo-cd/pull/15741 having the list of application afaik. When it is merged, i'll get back into it to make the final changes.

agaudreault avatar Mar 15 '24 15:03 agaudreault

https://github.com/argoproj/argo-cd/pull/15741 just got merged!

Tyrael avatar Apr 23 '24 14:04 Tyrael

@agaudreault any update on this? no pressure but I'm really waiting for this feature!

Tyrael avatar May 28 '24 20:05 Tyrael

@agaudreault any update on this? no pressure but I'm really waiting for this feature!

I should be able to resume the work next week so it is in the next release :)

agaudreault avatar May 28 '24 20:05 agaudreault

This feature was marked as having been completed in 2.12, but it is not anywhere on the 2.12 branch. I think we need to move it to 2.13 now.

jakedgy avatar Aug 08 '24 22:08 jakedgy