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(xo-web/backup): long-term retention settings #8141

Merged
merged 9 commits into from
Nov 28, 2024

Conversation

pdonias
Copy link
Member

@pdonias pdonias commented Nov 19, 2024

Requires #7999

Screenshots

Capture_2024-11-21_13:51:41

Description

Backup/New: UI for #7999.

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

  • develop skills of junior reviewers
  • limit the workload for senior reviewers
  • limit the number of unnecessary changes by the author
  1. The author creates a PR.
  2. Review process:
    1. The author assigns the junior reviewer.
    2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
    3. The junior reviewer assigns the senior reviewer.
    4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
  3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

  1. The author can request a review at any time, even if the PR is still a Draft.
  2. In theory, there should not be more than one reviewer at a time.
  3. The author should not make any changes:
    • When a reviewer is assigned.
    • Between the junior and senior reviews.

@pdonias pdonias force-pushed the pierre-long-term-retention branch from 8220394 to f141429 Compare November 19, 2024 13:16
@pdonias pdonias requested a review from MathieuRA November 19, 2024 13:17
@MathieuRA MathieuRA requested review from MelissaFrncJrg and removed request for MathieuRA November 19, 2024 13:25
@julien-f julien-f self-requested a review November 19, 2024 14:30
@julien-f
Copy link
Member

Very nice to see progress on this front, I'm eager to see this release 🙂

But, IMHO, this feature is not trivial to grasp and I feel like releasing it as is may trigger a lot of questions in the forum and support, like:

  • how it interacts with schedule's retentions
  • which backups are selected

I think it would be interesting to discuss with @fbeauchamp and @thomas-dkmt and to figure out how to improve, a few suggestions from the top of my head:

  • a small description above or below the limits
  • tooltips
  • a documentation page

IMO, keep it as simple as possible but give enough context to avoid too many questions 🙂

@julien-f julien-f removed their request for review November 21, 2024 09:19
@pdonias
Copy link
Member Author

pdonias commented Nov 21, 2024

Individual tooltips might not be necessary if there's a good description at the top of the card? (@fbeauchamp any idea what the description could be?)
And then we can indeed add a "Learn more" link and add more details in the documentation.

packages/xo-web/src/xo-app/backup/new/index.js Outdated Show resolved Hide resolved
packages/xo-web/src/xo-app/backup/new/index.js Outdated Show resolved Hide resolved
packages/xo-web/src/xo-app/backup/new/index.js Outdated Show resolved Hide resolved
packages/xo-web/src/xo-app/backup/new/index.js Outdated Show resolved Hide resolved
@MathieuRA
Copy link
Member

MathieuRA commented Nov 26, 2024

IMO, long term retention information should be added in the notes column of Backup/Preview/Backup jobs
Also, don't forget to add timezone and longTermRetention in api/backup-ng.mjs#SCHEMA_SETTINGS

@pdonias pdonias force-pushed the pierre-long-term-retention branch from f141429 to cf003f6 Compare November 28, 2024 11:33
@pdonias pdonias marked this pull request as ready for review November 28, 2024 11:33
@pdonias pdonias requested a review from MathieuRA November 28, 2024 11:33
@MathieuRA MathieuRA merged commit 07f8468 into master Nov 28, 2024
1 check passed
@MathieuRA MathieuRA deleted the pierre-long-term-retention branch November 28, 2024 11:34
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.

4 participants