-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
feat: use resource actions for auto-sync toggle instead of update permission (#21564) #22572
base: master
Are you sure you want to change the base?
feat: use resource actions for auto-sync toggle instead of update permission (#21564) #22572
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
2e61267
to
71301ca
Compare
This would be a breaking change... What about only adding a toggle auto-sync action on the application, but not changing the logic of the internal button? |
Would be nice to have consistent permissioning in the UI... Maybe the button could try the new API first and fall back. The backend could log a deprecation warning for the old API (assuming it's its own endpoint). |
Thanks for the feedback! following your suggestions: possible approaches are:
I'm leaning towards the Minimal Change Approach because:
Since this is a security-related change that affects user permissions, I'd like to get your thoughts on which approach would be better for the Argo CD community. What do you think? |
I think the approach @crenshaw-dev suggested has value because it allows to test the breaking changes and to opt-in the feature. You can do 2 separate PRs so it is easier to review. Implement approach 1, merge, then implement approach 2. I would add a requirement to 2)
|
1854c28
to
051a320
Compare
yea good point doing it as an opt-in feature. I've modified this PR to follow the first approach:
Once this PR is merged, I'll create a new PR for the second approach that will:
Since the UI changes involve feature flags and deprecation warnings, should I create a new issue or link to #21564? |
051a320
to
1cdd636
Compare
…mission (argoproj#21564) Signed-off-by: ebracha <[email protected]>
1cdd636
to
b8957ae
Compare
Checklist:
Description:
Closes #21564
This PR changes how auto-sync toggle is implemented in Argo CD by:
The change improves security by:
Testing
Action Discovery
The new
toggle-auto-sync
action is properly discovered for all application types:Action Execution
The action can be executed on any application:
# Toggle auto-sync on an application $ argocd app actions run helm-guestbook toggle-auto-sync --kind Application
UI Execution
The UI now uses the same action-based approach for toggling auto-sync:
toggle-auto-sync
actionRBAC Configuration
The new action-based approach allows for more granular RBAC control using Argo CD's policy format:
This configuration:
get
permission)