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): ability to block/unblock migration #8129

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

Conversation

stephane-m-dev
Copy link
Contributor

@stephane-m-dev stephane-m-dev commented Nov 13, 2024

Description

VM : add ability to block / unblock migration

XO-389
https://project.vates.tech/vates-global/projects/70ab2907-1ac3-4e7d-831f-a8752c36474d/issues/b8c25c6d-40ba-4cb6-8a6c-33d887692525

Capture d’écran du 2024-11-13 12-10-44
Capture d’écran du 2024-11-13 12-10-12

UPDATED : change TabButton to Toggle

Capture d’écran du 2024-11-14 17-39-40

UPDATED : add confirm modal on Toggle

Capture d’écran du 2024-11-15 17-09-12

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.

@stephane-m-dev stephane-m-dev changed the title feat(xo-server/xo-web): ability to block migration feat(xo-web): ability to block migration Nov 13, 2024
packages/xo-web/src/common/xo/index.js Outdated Show resolved Hide resolved
@@ -780,6 +784,13 @@ export default class TabAdvanced extends Component {
labelId='vmWarmMigration'
tooltip={isWarmMigrationAvailable ? undefined : _('availableXoaPremium')}
/>
<TabButton
btnStyle='warning'
handler={isMigrationAllowed ? vmBlockMigration : vmAllowMigration}
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand this, it would let users click the "Allow migration" button even when migrations are blocked for another reason (i.e. when vm.blockedOperations.migrate_send === 'another reason than "VM migration is blocked during backup"'), which means clicking the button won't change anything.
Maybe we can gray out the button in such case, or display an information message?

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to prevent the user from unblocking the migration, even if it has been blocked by the backup.

For instance, there might be an issue with the backup which prevented it from unblocking the operation at then end, and we want users to be able to bypass this.

But I think it would make sense to display the current reason it is blocked before unblocking, maybe by showing a modal (and only if the reason is not trivial, i.e. 'true').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julien-f Can we already ship the feature like this, and maybe improve it later?
Or do you want it to be improved now?

Copy link
Member

Choose a reason for hiding this comment

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

I fear that doing it in two steps will in fact make it more work and take longer.

IMHO that adding a quick confirm should be quick, what do you think @pdonias (of both the idea and how to implement it)?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. It gives more feedback and doesn't prevent the user from doing anything while keeping it safe. I think a simple confirm modal in vmAllowMigration that shows the reason is enough.

Copy link
Member

Choose a reason for hiding this comment

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

@pdonias Do you think that someone could help on the front-side?

@stephane-m-dev stephane-m-dev changed the title feat(xo-web): ability to block migration feat(xo-web): ability to block/unblock migration Nov 13, 2024
handlerParam={vm}
icon='vm-allow-migration'
labelId={isMigrationAllowed ? 'vmBlockMigration' : 'vmAllowMigration'}
/>
Copy link
Member

Choose a reason for hiding this comment

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

It seems I lost a comment there: use a toggle button like other blocked operations.

What I previously meant is: do not create a function that will switch the state as it can behave strangely if the cache is not up-to-date, the customer should be able to see whether it will block or unblock when clicking on the button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did: I split my toogle function into two different functions, and the button indicates whether it is used to block or unblock (as shown on the screenshots at the top of the PR).
What's wrong for you?

Copy link
Member

Choose a reason for hiding this comment

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

I want this feature to be displayed the same way other blocked operations are displayed:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julien-f OK it's clearer with a picture! I'll change that.

@stephane-m-dev stephane-m-dev force-pushed the ability-to-block-migration branch 3 times, most recently from da1b169 to 12d859b Compare November 15, 2024 09:18
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