feat(appset): implement ResourceTree for ApplicationSets

[alexymantha]

Part of the effort to add support for ApplicationSets in the UI, adds a ResourceTree endpoint similar that will be used to display the tree of an ApplicationSet and its children Applications.

As discussed in the linked issue, this uses the ApplicationSet status to track the children and the tree is built based on the data contained in the status.

The progressive sync feature added an ApplicationSetApplicationStatus field that is used to track the status and progress of the sync of the children's applications. I tried to reuse what the progressive sync was already doing. See comments for more details on the refactor.

Closes #15697

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. (UI will be implemented in another PR)
• [ ] Does this PR require documentation updates?
• [ ] I've updated documentation as required by this PR.
• [x] Optional. My organization is added to USERS.md.
• [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.

[reggie-k]

Thanks for being so quick with this! Can you elaborate about the progressive sync changes?

[alexymantha]

Can you elaborate about the progressive sync changes?

Yes! I tried to reuse the ApplicationSetApplicationStatus that was previously only used for the progressive sync so I had to make some changes to the progressive sync:

• moved some of the logic that was handling the status update outside of the feature flag.
• ~~moved the progressivesync-specific fields to a subfield of theApplicationSetApplicationStatus.~~
• change to use a status map instead of a slice to make it easier to update the status along the way since it's not only being updated by the progressive sync anymore.

Most of these changes could be avoided if I didn't try to reuse ApplicationSetApplicationStatus but I think it makes sense to make the status more generic and not depend on the progressive sync.

[reggie-k]

Thanks. Let's wait for Michael's review. And I agree that only nodes are relevant for AppSet, orphan nodes are not.

On Sun, Oct 1, 2023, 08:19 Alexy Mantha @.***> wrote:

Can you elaborate about the progressive sync changes?

Yes! I tried to reuse the ApplicationSetApplicationStatus that was previously only used for the progressive sync so I had to make some changes to the progressive sync:

• moved some of the logic that was handling the status update outside of the feature flag.
• moved the progressivesync-specific fields to a subfield of the ApplicationSetApplicationStatus.
• change to use a status map instead of a slice to make it easier to update the status along the way since it's not only being updated by the progressive sync anymore.

Most of these changes could be avoided if I didn't try to reuse ApplicationSetApplicationStatus but I think it makes sense to make the status more generic and not depend on the progressive sync.

— Reply to this email directly, view it on GitHub https://github.com/argoproj/argo-cd/pull/15741#issuecomment-1741962637, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEVDWBGTB6OFNUX4NDEHJEDX5D4MVANCNFSM6AAAAAA5NG7IHE . You are receiving this because you commented.Message ID: @.***>

[reggie-k]

@alexymantha is this waiting for a review or still in progress?

[alexymantha]

is this waiting for a review or still in progress?

Kinda both, I still need to refactor some of the tests to fit the new structure of the ApplicationStatus but it'll take a while so I would appreciate a confirmation of whether or not we want to go this route or if we should use a completely new status before I do the refactor of the tests.

For the feature itself, it's mostly done

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 76.66667% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 49.56%. Comparing base (f87897c) to head (1b065cb). Report is 81 commits behind head on master.

:exclamation: Current head 1b065cb differs from pull request most recent head f13b942. Consider uploading reports for the commit f13b942 to get more accurate results

Files	Patch %	Lines
...cationset/controllers/applicationset_controller.go	76.31%	14 Missing and 4 partials :warning:
server/applicationset/applicationset.go	85.00%	4 Missing and 2 partials :warning:
.../apis/application/v1alpha1/applicationset_types.go	0.00%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15741      +/-   ##
==========================================
- Coverage   49.73%   49.56%   -0.17%     
==========================================
  Files         274      271       -3     
  Lines       48948    47842    -1106     
==========================================
- Hits        24343    23712     -631     
+ Misses      22230    21789     -441     
+ Partials     2375     2341      -34     

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

[alexymantha]

I think the PR is ready for feedback. I'm also working on adding an E2E to test the status updates

[reggie-k]

@alexymantha can you update this branch from upstream master?

[alexymantha]

@reggie-k It should be good to go!

[alexymantha]

Hi @reggie-k! I added the missing group and kind, can you confirm if all the missing info is there? If so, this PR should be ready for a review.

[reggie-k]

Thanks a lot! I will test the changes. As per the discussion with @crenshaw-dev, I will merge these changes into my original PR, also the other PRs you are working on, so that everything gets reviewed at once.

Can you also add changes in the CLI for the AppSet resourc-tree?

On Thu, Jan 4, 2024, 19:05 Alexy Mantha @.***> wrote:

Hi @reggie-k https://github.com/reggie-k! I added the missing group and kind, can you confirm if all the missing info is there? If so, this PR should be ready for a review.

— Reply to this email directly, view it on GitHub https://github.com/argoproj/argo-cd/pull/15741#issuecomment-1877460879, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEVDWBARUU4ZJTU7STBMZKTYM3OMLAVCNFSM6AAAAAA5NG7IHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZXGQ3DAOBXHE . You are receiving this because you were mentioned.Message ID: @.***>

[reggie-k]

As per discussion with @crenshaw-dev, relevant CLI changes can be added later on separately.

[reggie-k]

Verified - the missing fields are now present, working as expected. Thanks @alexymantha!

[csantanapr]

what are the next steps to merge this @alexymantha and @reggie-k, just curious on this because there is another PR I'm interested https://github.com/argoproj/argo-cd/pull/16781

[alexymantha]

IIUC @crenshaw-dev suggested to merge it alongisde https://github.com/argoproj/argo-cd/pull/15731 to make it easier for the reviews but this PR is standalone and could be merged now if needed. It's up to the maintainers

[alexymantha]

Sorry @alexymantha it needs merge conflicts fixed again.

Done!

[Tyrael]

hi, what is the status here? any reason why this isn't merged yet?

[crenshaw-dev]

Would you mind rebasing and re-running codegen?

[alexymantha]

Thanks @crenshaw-dev! Everything is up-to-date now

[Tyrael]

Thanks!

[reggie-k]

Thank you @alexymantha and @crenshaw-dev!