-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: Progress Sync Unknown in UI #24202
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
fix: Progress Sync Unknown in UI #24202
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
|
@reggie-k, can you please review this for me! Thnx |
nitishfy
left a comment
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.
Thank you for raising the PR. Can you please attach a demonstration video if possible?
updatedFix.mov |
d3f364a to
aa63630
Compare
| const hasRollingSyncEnabled = (application: models.Application): boolean => { | ||
| // Only show Progressive Sync for applications that have an ApplicationSet owner reference | ||
| // The actual strategy validation is done in the ProgressiveSyncStatus component | ||
| return application.metadata.ownerReferences?.some(ref => ref.kind === 'ApplicationSet') || false; | ||
| }; |
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.
Hi @aali309
It’s me from the reported issue: #24190 (comment) :)
Maybe I’m missing something, but wouldn’t it be more fitting to rename this function to hasApplicationSetParent or isPartOfApplicationSet or something similar?
The function only checks if there’s an owner reference pointing to an ApplicationSet, and does not check if rolling sync has been enabled. I understand that the sync-strategy is checked later in the code, but I would argue that the current function name is a bit misleading considering what it actually does.
We are currently hiding that icon. What about the case when it has been enabled and shouldn't be unknown? |
current logic, I believe it's working correctly: |
577ea25 to
91247cd
Compare
|
@aali309 Could you attach a screenshot of how it looks like when the App is part of a progressive sync but the user has no permissions on the AppSet? I'm asking myself for this case, should we even display the progressive sync status for the App? |
91247cd to
e09721f
Compare
I think its better to create a follow up PR to close this access denied issue #24225 I was also thinking to have something like this For user awareness, it informs users that there is progressive sync information available, but they don't have access to it. WDYT? |
|
I like this approach, but not sure, from security point if view, that we are not disclosing info that isn't supposed to be disclosed. Would love to hear @crenshaw-dev or @agaudreault 's take on this. I'm adding it to today's contributor agenda meeting for discussion. |
Thank you @reggie-k , so I guess we are disabling it |
|
@aali309 Please see the summary from today's contributor meeting (the RBAC + docs stuff can go into another PR) |
|
I believe we would need the following scenarios tested:
|
5777196 to
1451cee
Compare
Tested all these 5 scenarios @reggie-k @agaudreault PTAL. |
| const appSetRef = application.metadata.ownerReferences.find(ref => ref.kind === 'ApplicationSet'); | ||
| const isPermissionError = (error: any): boolean => { | ||
| const errorText = extractErrorText(error); | ||
| return errorText.includes('Failed to load data'); |
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.
Relying on error text is fragile, could you look into the error object when permissionError occurs to see if something like errorCode or statusCode(403) or error type exists and use that instead?
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.
This is what I thought initially but here's what I found:
- The DataLoader component wraps HTTP errors in React elements, so we can't directly access HTTP status codes like error.status or error.statusText.
Therefore:
error.status → undefined (not accessible)
error.statusText → undefined (not accessible)
error.response → undefined (not accessible)
error.code → undefined (not accessible)
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.
It will be better to check for permissions explicitly using can-i, like here:
argo-cd/ui/src/app/applications/components/resource-details/resource-details.tsx
Line 284 in 993344e
| const logsAllowed = await services.accounts.canI('logs', 'get', application.spec.project + '/' + application.metadata.name); |
Showing / hiding logs of pods in the App is done with a similar logic - shown if the user has permissions, hidden otherwise.
ui/src/app/applications/components/application-status-panel/application-status-panel.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-status-panel/application-status-panel.tsx
Outdated
Show resolved
Hide resolved
ranakan19
left a comment
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.
left some comments inline
6235ef9 to
9afdc12
Compare
|
@reggie-k |
710808c to
a463e21
Compare
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]>
a463e21 to
c20ec1d
Compare
keithchong
left a comment
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.
LGTM
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.
Merging.
Signed-off-by: Atif Ali <[email protected]>
|
🍒 Cherry-pick PR created for 3.2: #24641 |
Signed-off-by: Atif Ali <[email protected]>
|
🍒 Cherry-pick PR created for 3.1: #24643 |
Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]> Co-authored-by: Atif Ali <[email protected]>
Signed-off-by: Atif Ali <[email protected]> Co-authored-by: Atif Ali <[email protected]>



Closes: #24190
Closes: #24296
Checklist: