Skip to content
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(UI): add enabled field to explicitly set in automatedSync in SyncPolicy #22482

Merged
merged 6 commits into from
Apr 9, 2025

Conversation

aali309
Copy link
Contributor

@aali309 aali309 commented Mar 25, 2025

Adding enable field to explicitly set in automatedSync in SyncPolicy on UI

Fixes #21647
Part of: #21999

This PR add a boolean field named enable in Spec.SyncPolicy.Automated for explicitly enabling or disabling automatedSync.
These changes don't affect the current behaviour.
This field can be set using

spec:
  syncPolicy:
    automated:
      enabled: true

Checklist:

  • 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.
  • 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.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Copy link

bunnyshell bot commented Mar 25, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@aali309 aali309 changed the title feat(UI): add enable field for automatedSync feat(UI): add enabled field to explicitly set in automatedSync in SyncPolicy Mar 25, 2025
@aali309 aali309 marked this pull request as ready for review March 25, 2025 17:41
@aali309 aali309 requested a review from a team as a code owner March 25, 2025 17:41
@aali309
Copy link
Contributor Author

aali309 commented Mar 25, 2025

@anandrkskd can you please take a look at this, its part of what you did on the UI side. cc @anandf

@aali309 aali309 force-pushed the sync-policy-enabled branch from 536b83c to 83bd592 Compare March 25, 2025 22:48
@agaudreault
Copy link
Member

@aali309 For UI PRs, please provide UI screenshot or videos of the previous vs. new experience.

@aali309 aali309 force-pushed the sync-policy-enabled branch from 83bd592 to 8d14694 Compare March 26, 2025 18:47
@aali309
Copy link
Contributor Author

aali309 commented Mar 26, 2025

Creating applications before:
Screenshot 2025-03-26 at 2 56 37 PM

Creating applications After:
Screenshot 2025-03-26 at 2 51 17 PM
Application summary view before:
Screenshot 2025-03-26 at 2 55 57 PM

Application summary view after:
Screenshot 2025-03-26 at 2 53 33 PM

@aali309 aali309 force-pushed the sync-policy-enabled branch from 8d14694 to e64defb Compare April 1, 2025 17:10
@anandrkskd
Copy link
Contributor

While trying a test case, which might be confusing for user

  • If the existing user has enabled the automated Sync using .spec.syncPolicy.automated: {}, UI shows the checkbox for ENABLE AUTO-SYNC is not ticked.

  • It is same, if the field .spec.syncPolicy.automated.enabled: false is set, the UI shows the same as the above

  • Autosync enabled using .spec.syncPolicy.automated: {}
    image

  • autoSync disabled using .spec.syncPolicy.automated.enabled: false
    image

@anandrkskd
Copy link
Contributor

anandrkskd commented Apr 2, 2025

Is this an intended behaviour?
when the enabled field is set to false, whenever we are making changes using UI to enable the prune resource or self heal fields the enabled field is reset to true

Screen.Recording.2025-04-02.at.2.43.39.PM.mov

@aali309 this seems like a bug, can we confirm this behaviour?

@aali309 aali309 force-pushed the sync-policy-enabled branch 2 times, most recently from 586fa29 to a8e8fad Compare April 2, 2025 20:24
@aali309
Copy link
Contributor Author

aali309 commented Apr 3, 2025

Is this an intended behaviour? when the enabled field is set to false, whenever we are making changes using UI to enable the prune resource or self heal fields the enabled field is reset to true

Screen.Recording.2025-04-02.at.2.43.39.PM.mov
@aali309 this seems like a bug, can we confirm this behaviour?

Yes, its a bug, fixed now. THNX @anandrkskd

@aali309 aali309 force-pushed the sync-policy-enabled branch from c51aad9 to 72744f2 Compare April 3, 2025 17:43
@anandrkskd
Copy link
Contributor

I have checked the changes and seems to be behaving as expected, thanks @aali309.
LGTM

@aali309 aali309 force-pushed the sync-policy-enabled branch 2 times, most recently from a44c1b0 to 75e7f7f Compare April 7, 2025 18:37
aali309 added 3 commits April 9, 2025 12:55
Signed-off-by: Atif Ali <atali@redhat.com>
Signed-off-by: Atif Ali <atali@redhat.com>
Signed-off-by: Atif Ali <atali@redhat.com>
@aali309 aali309 force-pushed the sync-policy-enabled branch from 75e7f7f to ab0adc7 Compare April 9, 2025 16:55
@aali309 aali309 requested a review from keithchong April 9, 2025 18:33
aali309 added 2 commits April 9, 2025 15:14
Signed-off-by: Atif Ali <atali@redhat.com>
Signed-off-by: Atif Ali <atali@redhat.com>
Copy link
Contributor

@keithchong keithchong left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for opening #22592 to follow-up on improving the usability of this panel.

@keithchong keithchong merged commit 999dbb2 into argoproj:master Apr 9, 2025
23 checks passed
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.

A dedicated field to enable or disable application syncPolicy
4 participants