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

Fleet: expose Polling Interval for GitRepos #13372

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

torchiaf
Copy link
Member

@torchiaf torchiaf commented Feb 11, 2025

Summary

Fixes #7824

Occurred changes and/or fixed issues

We are adding the possibility to:

  • enable/disable polling.
  • set up the pollingInterval value when creating/editing GitRepos.

Technical notes summary

  • It should be possible to enable/disable polling
  • It should be possible to assign the polling interval in edit mode and display the value in view mode
  • Default Polling Interval should be 60 seconds when creating new GitRepo
  • The value in the input field should be 15 seconds when editing a GitRepo and spec.pollingInterval is undefined
  • A warning should be displayed in case of the polling interval is lower than 15 seconds
  • A warning should be displayed in case of a webhook is configured https://fleet.rancher.io/webhook

Areas or cases that should be tested

GitRepo, edit page

Areas which could experience regressions

Screenshot/Video

Create/Edit

Screencast.from.2025-02-13.17-48-28.webm

View

image

Tooltip

image

Warning, the interval is lower than the recommended value

image

Warning, a webhook is configured

image

Enable/Disable Polling action

image

image

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@@ -2464,6 +2464,12 @@ fleet:
keepResourcesBanner: When enabled above, resources will be kept when deleting a GitRepo or Bundle - only Helm release secrets will be deleted.
correctDrift: Enable Self-Healing
correctDriftBanner: When enabled, Fleet will ensure that the cluster resources are kept in sync with the git repository. All resource changes made on the cluster will be lost.
syncronization:
Copy link
Member Author

@torchiaf torchiaf Feb 11, 2025

Choose a reason for hiding this comment

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

@manno could you please help reviewing the text messages ? I'm not sure if Syncronization as section title is appropriate and tooltip/warning messages could be improved.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest "polling", that's what we use for the fields and in the documentation. It describes the cyclic requests well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks.

@manno
Copy link
Member

manno commented Feb 11, 2025

I would suggest to use a larger default values, like a minute. It depends on the installation, but larger fleets might use an interval of 1 hour.

Note, there is some interaction with https://fleet.rancher.io/webhook

If you configured the webhook the polling interval will be automatically adjusted to 1 hour.

Additionally, setting paused: true on the GitRepo does not disable polling. To disable polling completely users can set
disablePolling: true (https://fleet.rancher.io/ref-gitrepo).

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

@edenhernandez-suse Are we sure we want to stick on another setting to this step in the wizard? the page is already pretty tall

It feels like we should redistribute components into modified steps

  1. basics - name / namespace, labels, annotations
  2. source specific - repo / branch, path, git auth
  3. functional specific - syncronization, helm auth, TLS verification, resource handling, soon oci repo details
  4. cluster target details

@manno do those groups make sense?

Edit: We also need to optimise for the simple case, whilst still allowing users to configured more advanced settings.

Maybe the best option is to have step 3 as Advanced Options? so steps relabelled and organised would be...

  1. Resource - name / namespace, labels, annotations
  2. Source - repo / branch, path, git auth
  3. Target - cluster selection / selector
  4. Advanced - syncronization, helm auth, TLS verification, resource handling, soon oci repo details (order tbd)

@torchiaf torchiaf force-pushed the 7824-expose-polling branch from e2efd82 to 7725e8a Compare February 12, 2025 15:22
@torchiaf
Copy link
Member Author

@richard-cox @edenhernandez-suse I'd move the wizard refactoring in another issue/PR, since this change it's becoming huge considering that we have to add the Disable Polling field and weebhook detection.

@richard-cox
Copy link
Member

@torchiaf ok. can you create an issue to address the vertical height of fields in that first step, and reference my comment as a possible solution?

@torchiaf torchiaf force-pushed the 7824-expose-polling branch 3 times, most recently from 5a1f4cc to 9134387 Compare February 13, 2025 16:04
@torchiaf
Copy link
Member Author

@richard-cox @edenhernandez-suse I created this one that summarize steps and fields size improvements: #13402

- Add UnitInput field to edit/display the pollingInterval
- Add unit tests
- Add warning banners to inform about polling interval low value risks
- Scroll page to show warning banners

Signed-off-by: Francesco Torchia <[email protected]>
Signed-off-by: Francesco Torchia <[email protected]>
@torchiaf torchiaf force-pushed the 7824-expose-polling branch from 9134387 to 98e5c50 Compare February 17, 2025 16:03
Signed-off-by: Francesco Torchia <[email protected]>
@torchiaf torchiaf mentioned this pull request Feb 20, 2025
7 tasks
@torchiaf torchiaf requested review from richard-cox and edenhernandez-suse and removed request for aalves08 February 21, 2025 08:57
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.

Expose polling rate for GitRepos in chart in Rancher UI
4 participants