argo-cd
argo-cd copied to clipboard
feat: AppSet UI Support
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.
- [ ] The title of the PR states what changed and the related issues number (used for the release note).
- [ ] The title of the PR conforms to the Toolchain Guide
- [ ] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
- [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
- [ ] Does this PR require documentation updates?
- [ ] I've updated documentation as required by this PR.
- [ ] Optional. My organization is added to USERS.md.
- [X] I have signed off all my commits as required by DCO
- [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
- [ ] My build is green (troubleshooting builds).
- [ ] 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.
Adding native support for AppSet on the UI. This is an initial draft version. AppSets currently are located under Settings in the UI. They are treated as app of apps. The basic idea - thanks @keithchong! - there is an AbstractApp that has fields common to Apps and Appsets, and then App and AppSet extend it and only have their specific fields. And so on for filters, preferences, etc.
Current limitations:
- For components like application-list, the component does not know which specific app type to return, it only depends on whether the list was clicked on from /applications or from /settings/applicationsets. Meaning depends on the path. Determining by the path does not work yet, currently it is hard coded to assume the path was from /applicationsets. To observe the behavior for Applications, like today, can be done by changing the false value to true. Temporarily, only AppSets are returned even when getting to /applications to observe the new functionality.
- ResourceTree for AppSet does not exist on the backend, it is being developed, so the AppSet resource tree view is empty. #15697
- Stream (watch) does not exist on backend side, it is being developed. Currently a 404 for stream is received and visible with developer tools. #15695
- For AppSet details view, currently only manifest and events tabs are available. Events are not retrieved currently cause need to be implemented on the backend side. #15753
- Currently, AppSet is not planned for editing or creating in the UI.
- Currently, the health of AppSet is calculated as the first condition status, this is temporary and would require populating the health status at the backend side and caching it to Redis. Will open a separate issue for this one.
- The VSCode auto-formatting argues with the ui linter running in the checks pipeline, so currently the ui linting step fails. Any ideas how to fix this while being able to format the typescript code in VSCode?
@reggie-k Do you want to add some screenshots of the expected outcome in the comments? Just makes it easier for discussion.
Sure, @todaywasawesome, hope those help!
-
Until the dynamic context discovery would be implemented (discover whether invoked from /applications or from /settings/applicationsets), this is displayed for clicking on both Applications on the left and on Settings/Application:
Meaning, currently in the PR only ApplicationSets are displayed (not Apps). With the dynamic context in place, clicking on Applications on the left will display Application list, and clicking on Settings/ApplicationSets will display ApplicationSet list
-
Currently empty ResourceTree for AppSet:
-
Stream 404, visible in DevTools:
-
Events not retrieved yet:
Current status of this (rather complex) feature and general concepts:
- AppSets currently are located under Settings in the UI.
- AppSets are treated as app of apps.
- The basic idea - thanks @keithchong! - there is an AbstractApp that has fields common to Apps and Appsets, and then App and AppSet extend it and only have their specific fields. And so on for filters, preferences, etc.
- For components like application-list, and some others, the decision whether to display Apps or AppSets is only based on the location path. For other components, it is kind-based decision (whether to display an App or an AppSet based on the kind of the AbstractApp). For preferences, I still don't have an elegant path-based decision since could not use the location path there, not sure how it affects the functionality though, still investigating.
- ResourceTree for AppSet did not exist on the backend, it is needed for the resource tree view. #15697 Attaching screenshot of how it looks in the UI based on the backend changes. This backend functionality is mostly finished, with some last minor changes.
- Stream (watch) does not exist on backend side, it is being developed. Currently a 404 for stream is received and visible with developer tools. #15695
- Events for AppSet did not exist on the backend, it is needed for the AppSet details view. #15753 This backend functionality is finished and ready for review.
- Currently, AppSet is not planned for editing or creating in the UI.
- Currently, the health of AppSet is calculated as the last condition status, this is temporary and would require populating the health status at the backend side and caching it to Redis. Will open a separate issue for this one.
- Some of the automatic UI tests are failing, investigating why.
- All the important UI components for listing, searching, filtering, displaying details and resource tree for AppSet are working correctly, I also manually tested the regression on the Apps behavior and Apps are working correctly as well, but surely more thorough testing would be needed since I basically refactored about 50% of the ArgoCD UI :)
- We should start thinking about a nice icon for AppSet :) One idea is the App icon surrounded by { } for the mathematical representation of a set (credit to https://github.com/dudinea).
Thanks @alexymantha for implementing the missing backend functionality!
- For components like application-list, and some others, the decision whether to display Apps or AppSets is only based on the location path. For other components, it is kind-based decision (whether to display an App or an AppSet based on the kind of the AbstractApp). For preferences, I still don't have an elegant path-based decision since could not use the location path there, not sure how it affects the functionality though, still investigating.
Hi @reggie-k, I made some changes so we don’t have to rely on checking the location path. So we don’t need to use ctx.history.location.pathname and we can probably get rid of isInvokedFromAppsPath() in utils.tsx.
I’ve rebased your changes so they are compatible with the latest master in the repo. See the branch in this PR https://github.com/argoproj/argo-cd/pull/16801
Also, my changes will be on top of this branch so you will see what I've done.
There are two conflicting files so I merged these manually so a more careful review should be done on these two files.
Conflicting files ui/src/app/applications/components/application-status-panel/application-status-panel.tsx ui/src/app/shared/services/extensions-service.ts
The list view did not include the name of the app set so I added this in. I put a temporary placeholder icon there until we get a final icon, like you mentioned.
Hi @reggie-k , check this commit here. The idea is that we already know what each List 'page' is for (one for the list of Applications, and the other for the list of ApplicationSets). So, we should just make use of that piece of information and propagate that property onward/down. Please review and if you like it, you can merge it into your branch. Note that this commit does include all of the changes to get rid of the isInvokedFromAppsPath(), so please have a look at this first. Also, I think we could probably get rid of isApp() too.
https://github.com/argoproj/argo-cd/pull/16801/commits/0143f4db7aa3dbc3bfe56b7d69703ca5230259bc
Let me know what you think.
@reggie-k , have a look at this commit. This refactors the details page such that it removes the need for isApp and isInvokedFromAppsPath. Currently, the ApplicationDetails is a class, so I'm just taking advantage of this for both applications and applicationsets. It's definitely not polished off, but wanted to give you an idea what the general direction is. We can further refine this if desired.
Codecov Report
Attention: Patch coverage is 60.24845%
with 64 lines
in your changes are missing coverage. Please review.
Project coverage is 49.78%. Comparing base (
f87897c
) to head (bdaacee
). Report is 36 commits behind head on master.
:exclamation: Current head bdaacee differs from pull request most recent head 3f02356. Consider uploading reports for the commit 3f02356 to get more accurate results
Files | Patch % | Lines |
---|---|---|
server/applicationset/applicationset.go | 60.86% | 31 Missing and 14 partials :warning: |
server/applicationset/broadcaster.go | 57.77% | 18 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## master #15731 +/- ##
==========================================
+ Coverage 49.73% 49.78% +0.05%
==========================================
Files 274 275 +1
Lines 48948 49140 +192
==========================================
+ Hits 24343 24465 +122
- Misses 22230 22286 +56
- Partials 2375 2389 +14
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Why application sets are under settings vs. having "Application Sets" in the left menu at the same level as Applications?
Why application sets are under settings vs. having "Application Sets" in the left menu at the same level as Applications?
I started with them being next to the where the Applications are, but Alexander M. asked to have them under the Settings as at the first phase they would be available to admins only.
Accidentally closed, will reopen from another branch.