Skip to content

feat(appset): add Refresh RPC and UI action (#27798)#27932

Open
AsifAd wants to merge 5 commits into
argoproj:masterfrom
AsifAd:feat/appset-refresh-ui-27798
Open

feat(appset): add Refresh RPC and UI action (#27798)#27932
AsifAd wants to merge 5 commits into
argoproj:masterfrom
AsifAd:feat/appset-refresh-ui-27798

Conversation

@AsifAd
Copy link
Copy Markdown
Contributor

@AsifAd AsifAd commented May 19, 2026

Summary

Fixes #27798.

  • Adds Refresh RPC on ApplicationSet service (POST /api/v1/applicationsets/{name}/refresh)
  • Server patches argocd.argoproj.io/application-set-refresh: "true" (controller already handles this)
  • UI Refresh action on AppSet list tile, table row, detail action menu, and resource details panel
  • Supports appsetNamespace for namespaced ApplicationSets

Test plan

  • go test ./server/applicationset/... — includes TestRefreshAppSet (default ns, named ns, forbidden ns, not found)
  • cd ui && pnpm test — 165 tests pass (incl. ApplicationSet refresh helpers)
  • ESLint on changed UI files
  • Manual: Kind + make start-local → Git-directory AppSet → Refresh from UI → verify annotation + reconcile (maintainer QA)

Expose ApplicationSet refresh via POST /api/v1/applicationsets/{name}/refresh
which patches argocd.argoproj.io/application-set-refresh. Adds Refresh
actions to AppSet list tile, table row, detail toolbar, and resource panel.

Signed-off-by: Asif Draxi <asifdraxi@gmail.com>
@AsifAd AsifAd requested a review from a team as a code owner May 19, 2026 19:05
@bunnyshell
Copy link
Copy Markdown

bunnyshell Bot commented May 19, 2026

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

AsifAd added 2 commits May 20, 2026 00:40
Pass refreshApplicationSet through ApplicationTiles and ApplicationsTable
so TypeScript lint passes when AppSets appear on /applications.

Signed-off-by: Asif Draxi <asifdraxi@gmail.com>
Empty commit to re-run flaky race unit test job.

Signed-off-by: Asif Draxi <asifdraxi@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@b035a77). Learn more about missing BASE report.
⚠️ Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
server/applicationset/applicationset.go 86.66% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #27932   +/-   ##
=========================================
  Coverage          ?   64.40%           
=========================================
  Files             ?      422           
  Lines             ?    57859           
  Branches          ?        0           
=========================================
  Hits              ?    37262           
  Misses            ?    17093           
  Partials          ?     3504           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

can you share the screenshot of the UI?

@AsifAd
Copy link
Copy Markdown
Contributor Author

AsifAd commented May 20, 2026

/bns:deploy

@AsifAd AsifAd requested a review from a team as a code owner May 20, 2026 08:02
@AsifAd
Copy link
Copy Markdown
Contributor Author

AsifAd commented May 20, 2026

Thanks for the review @nitishfy — UI screenshots from local testing on this branch (v3.5.0+fc514e8):

ApplicationSets list — tiles view (Refresh on tile)
ApplicationSets tiles refresh

ApplicationSets list — table view (Refresh action)
ApplicationSets table refresh

ApplicationSet details — toolbar Refresh
ApplicationSet details refresh

ApplicationSet details — action menu (Refresh visible in toolbar)
ApplicationSet details actions

Refresh triggers POST /api/v1/applicationsets/{name}/refresh, which sets argocd.argoproj.io/application-set-refresh: "true" and the ApplicationSet controller reconciles.

@AsifAd AsifAd force-pushed the feat/appset-refresh-ui-27798 branch from d9f76a0 to fc514e8 Compare May 20, 2026 08:06
Copy link
Copy Markdown
Contributor

@ppapapetrou76 ppapapetrou76 left a comment

Choose a reason for hiding this comment

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

Can you please add a test for the retry mechanism and the waitSync after refresh?

Comment thread server/applicationset/applicationset.go Outdated
AsifAd added a commit to AsifAd/argo-cd that referenced this pull request May 21, 2026
Add conflict-retry, exhausted-retry, waitSync, and post-refresh waitSync
tests requested in argoproj#27932 review feedback.
@AsifAd AsifAd requested a review from a team as a code owner May 21, 2026 13:57
@AsifAd
Copy link
Copy Markdown
Contributor Author

AsifAd commented May 21, 2026

Addressed review feedback in 5629819:

Tests added (server/applicationset/applicationset_test.go):

  • TestRefreshAppSetWithConflicts — retry loop succeeds after an update conflict
  • TestRefreshAppSetTooManyConflicts — returns error after 10 conflicts
  • TestWaitSync — cache-up-to-date returns immediately; stale cache waits until timeout
  • TestRefreshAppSetWaitSyncrefreshAppSet calls waitSync after a successful update

All new tests pass locally (go test ./server/applicationset/ -run 'TestRefreshAppSetWithConflicts|TestRefreshAppSetTooManyConflicts|TestWaitSync|TestRefreshAppSetWaitSync').

Re @ppapapetrou76's backoff suggestion on the retry loop — kept the existing fixed-interval retry for now since it matches the Application refresh pattern; happy to revisit if you'd prefer exponential backoff.

AsifAd added 2 commits May 21, 2026 19:40
Add conflict-retry, exhausted-retry, waitSync, and post-refresh waitSync
tests requested in argoproj#27932 review feedback.

Signed-off-by: Asif Draxi <asifdraxi@gmail.com>
Align server refreshAppSet with webhook.refreshApplicationSet by using
retry.RetryOnConflict(retry.DefaultBackoff) instead of a tight 10-attempt
loop. Adjust exhausted-retry test for the new error shape.

Signed-off-by: Asif Draxi <asifdraxi@gmail.com>
@AsifAd AsifAd force-pushed the feat/appset-refresh-ui-27798 branch from 5629819 to 22b937f Compare May 21, 2026 14:10
@AsifAd
Copy link
Copy Markdown
Contributor Author

AsifAd commented May 21, 2026

Follow-up in 22b937f2a / 78836acab:

Backoff (review feedback): refreshAppSet now uses retry.RetryOnConflict(retry.DefaultBackoff, ...), matching applicationset/webhook.refreshApplicationSet for the same refresh annotation patch.

DCO: Added missing Signed-off-by on the test commit (78836acab).

Tests pass locally for the refresh/waitSync suite.

@AsifAd
Copy link
Copy Markdown
Contributor Author

AsifAd commented May 22, 2026

Follow-up on review feedback — all addressed and the open thread is resolved:

  • Backoff: refreshAppSet now uses retry.RetryOnConflict(retry.DefaultBackoff, ...) (22b937f), aligned with the webhook refresh path.
  • Tests: conflict retry loop, too-many-conflicts error, and waitSync coverage (78836ac).
  • CI: full matrix green on latest SHA.

@ppapapetrou76 @nitishfy — ready for another look when you have a moment. Thanks!

Copy link
Copy Markdown
Contributor

@ppapapetrou76 ppapapetrou76 left a comment

Choose a reason for hiding this comment

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

I'm not a UI expert so this PR needs a review from someone more familiar than I am

func (s *Server) Refresh(ctx context.Context, q *applicationset.ApplicationSetGetQuery) (*v1alpha1.ApplicationSet, error) {
namespace := s.appsetNamespaceOrDefault(q.AppsetNamespace)

appset, err := s.getAppSetEnforceRBAC(ctx, rbac.ActionUpdate, namespace, q.Name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The analogous Application hard-refresh uses rbac.ActionGet. Using ActionUpdate for a refresh-only action means read-only users or operators with get/sync privileges but not update will be blocked from triggering a refresh — which seems unintentional.

return updated, nil
}

func (s *Server) refreshAppSet(ctx context.Context, appset *v1alpha1.ApplicationSet) (*v1alpha1.ApplicationSet, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i believe there is already a refreshApplicationSet() inside webhook.go that uses RetryOnConflict as well
Could we re-use that one instead of re-implementing it here?

Maybe we can extract a shared helper method for each caller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Refresh button for ApplicationSets

4 participants