MTV-1726: Edit VMs list in migration plan #1414

jschuler · 2024-12-16T15:32:12Z

https://issues.redhat.com/browse/MTV-1726

Design story:
https://issues.redhat.com/browse/HPUX-166

Provide users the possibility to add more VMs (not just delete) to an existing plan.
See this table below for when it is possible to do so.
Basically, whenever you can update plan mappings, you can also update plan VMs.

For the mappings section, I reused the plan details mappings tab section, since it can patch existing mappings

Plan status	VMs status	Can select to cancel	Can select to remove	Can edit plan VMs (add/remove)	Can edit mappings	Notes
Running		TRUE	FALSE	FALSE	FALSE
Succeeded		FALSE	FALSE	FALSE	FALSE	no actions available
Failed	At least 1 VM migrated	FALSE	TRUE	TRUE (non-migrated only)	TRUE (non-migrated only)	can restart plan
Canceled	At least 1 VM migrated	FALSE	TRUE	TRUE (non-migrated only)	TRUE (non-migrated only)	can restart plan
Failed	No VM migrated	FALSE	FALSE	TRUE	TRUE
Canceled	No VM migrated	FALSE	FALSE	TRUE	TRUE
Ready		FALSE	FALSE	TRUE	TRUE
Error		FALSE	FALSE	TRUE	TRUE	No VMs were migrated

See video of before (green border)/after:

Screen.Recording.2024-12-13.at.12.04.40.PM.mov

jschuler · 2024-12-16T15:56:40Z

packages/forklift-console-plugin/src/components/page/StandardPageWithExpansion.tsx

@@ -0,0 +1,149 @@
+import React, { useState } from 'react';


this is basically a copy of packages/forklift-console-plugin/src/components/page/StandardPageWithSelection.tsx but without selection, only expansion

This variation is needed for Edit VMs case since there should be no checkboxes, just expandable rows.

jschuler · 2024-12-16T15:58:01Z

packages/forklift-console-plugin/src/modules/Plans/utils/types/PlanEditAction.ts

@@ -0,0 +1 @@
+export type PlanEditAction = 'PLAN' | 'VMS';


it will allow us next (with small changes like this) to edit plans too

jschuler · 2024-12-16T15:59:02Z

packages/forklift-console-plugin/src/modules/Plans/views/details/tabs/Mappings/PlanMappings.tsx

-  loadError: unknown;
+  loaded?: boolean;
+  loadError?: unknown;
+  planMappingsState: PlanMappingsSectionState;


I lifted the state up so that we can reuse planmappings from Edit VMs case

jschuler · 2024-12-16T16:02:45Z

packages/forklift-console-plugin/src/modules/Plans/views/edit/PlanEditPage.tsx

+
+import '../create/PlanCreatePage.style.css';
+
+export const PlanEditPage: React.FC<{


initially copied from PlanCreatePage, but there were enough changes to make it its own component

codecov-commenter · 2024-12-16T19:39:39Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.20%. Comparing base (13484d0) to head (0eafaf8).
Report is 184 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
- Coverage   36.81%   36.20%   -0.62%     
==========================================
  Files         158      159       +1     
  Lines        2548     2580      +32     
  Branches      599      616      +17     
==========================================
- Hits          938      934       -4     
- Misses       1428     1451      +23     
- Partials      182      195      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MTV-1686] Simplify/update migration plan status cell Signed-off-by: Joachim Schuler <[email protected]>

Signed-off-by: Joachim Schuler <[email protected]>

yaacov · 2024-12-29T06:09:08Z

@sgratch please review

@jschuler happy holidays, @sgratch the maintainer, asked to review big PRs so we will wait for her review.

Signed-off-by: Joachim Schuler <[email protected]>

metalice

Hey @jschuler

I went over the PR, following our best practices, and I didn't comment on everything as I trust that you will understand from the comments that I did add and adjust in all the places that it fit. If something isn't clear, just let me know over Slack

metalice · 2025-01-15T15:08:33Z

packages/forklift-console-plugin/src/modules/Providers/views/migrate/useUpdateEffect.ts

+  }, [state.flow.editingDone]);
+};
+
+async function updateMappings(


Seems redundant? Only moving args?

metalice · 2025-01-15T15:10:26Z

packages/forklift-console-plugin/src/modules/Providers/views/migrate/useUpdateEffect.ts

+import { setAPiError } from './reducer/actions';
+import { CreateVmMigrationPageState } from './types';
+
+const updatePlan = async (plan: V1beta1Plan) => {


please export to a util file, and improve to a meaningful naming, ex. updatePlanSpecVMS

metalice · 2025-01-16T11:39:16Z

packages/forklift-console-plugin/src/modules/Providers/views/migrate/useUpdateEffect.ts

+      return;
+    }
+
+    Promise.all([


can we convert to try catch with await ?

metalice · 2025-01-16T11:40:42Z

packages/forklift-console-plugin/src/modules/Providers/views/migrate/useUpdateEffect.ts

+  });
+};
+
+export const useUpdateEffect = (


why are we updaing state in this hook? do we really need this hook? its not returning anything

metalice · 2025-01-16T11:42:11Z

...es/forklift-console-plugin/src/modules/Providers/views/migrate/reducer/createInitialState.ts

          namespace,
        },
        spec: {
-          ...planTemplate?.spec,
+          ...plan?.spec,
          provider: {
            source: getObjectRef(sourceProvider),


no need for ...plan?.spec?.provider here?

metalice · 2025-01-16T15:52:04Z

packages/forklift-console-plugin/src/modules/Plans/views/details/tabs/Mappings/PlanMappings.tsx

+  const migratedVmIds = plan?.status?.migration?.vms?.reduce((migrated, vm) => {
+    if (getVMMigrationStatus(vm) === 'Succeeded') {
+      migrated.push(vm.id);
+    }
+    return migrated;
+  }, []);
+  const migratedVms = vmData.filter((vm) => migratedVmIds?.includes(vm.vm.id));
+  const networkIdsUsedByMigratedVms =
+    sourceProvider?.spec?.type !== 'ovirt' ? getNetworksUsedBySelectedVms(migratedVms, []) : [];
+  const storageIdsUsedByMigratedVms = ['ovirt', 'openstack'].includes(sourceProvider?.spec?.type)
+    ? []
+    : getStoragesUsedBySelectedVms({}, migratedVms, []);


useMemo to prevent rendering and exporting to a util file

metalice · 2025-01-16T15:52:51Z

packages/forklift-console-plugin/src/modules/Plans/views/details/components/MappingListItem.tsx

+            ) : (
+              <Button
+                onClick={onClick}
+                variant="plain"


u can use ButtonVariant from PF

metalice · 2025-01-16T15:54:06Z

packages/forklift-console-plugin/src/components/page/StandardPageWithExpansion.tsx

+ *   // ...other props
+ * />
+ */
+export function StandardPageWithExpansion<T>(props: StandardPageWithExpansionProps<T>) {


Please consist with arrow functions

metalice · 2025-01-16T15:54:32Z

packages/forklift-console-plugin/src/components/page/StandardPageWithExpansion.tsx

+  const { toId, onExpand, expandedIds, ...rest } = props;
+
+  if (onExpand && (!toId || !expandedIds)) {
+    throw new Error('Missing required properties: toId, expandedIds');


do we have other places in code where UI is thowring?

metalice · 2025-01-16T15:55:04Z

packages/forklift-console-plugin/src/components/page/StandardPageWithExpansion.tsx

+ * Contract:
+ * 1. IDs provided with toId() function are unique and constant in time
+ */
+export function withIdBasedExpansion<T>({


arrow function and please avoid nested components

Signed-off-by: Joachim Schuler <[email protected]>

sonarqubecloud · 2025-01-17T14:22:36Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
7.2% Duplication on New Code

See analysis details on SonarQube Cloud

jschuler mentioned this pull request Dec 16, 2024

MTV-1726: Add vms to existing plans #1406

Closed

jschuler requested a review from yaacov December 16, 2024 15:33

jschuler commented Dec 16, 2024

View reviewed changes

Merge pull request kubev2v#1411 from jpuzz0/MTV-1686-status-updates

003bc27

[MTV-1686] Simplify/update migration plan status cell Signed-off-by: Joachim Schuler <[email protected]>

jschuler force-pushed the mtv-1726_add-vms branch from 999f277 to 003bc27 Compare December 18, 2024 16:35

merge

86620ef

Signed-off-by: Joachim Schuler <[email protected]>

rebase | use k8spatch

90cb409

Signed-off-by: Joachim Schuler <[email protected]>

jschuler requested a review from sgratch January 10, 2025 20:08

jschuler added 2 commits January 10, 2025 16:18

simplify

4cf4327

Signed-off-by: Joachim Schuler <[email protected]>

Allow plan mappings to be modified even if a VM has migrated

0eafaf8

Signed-off-by: Joachim Schuler <[email protected]>

metalice reviewed Jan 16, 2025

View reviewed changes

sgratch added the enhancement Categorizes issue or PR as related to a new feature. label Jan 17, 2025

handle edge case of VM not found

a95afef

Signed-off-by: Joachim Schuler <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MTV-1726: Edit VMs list in migration plan #1414

MTV-1726: Edit VMs list in migration plan #1414

jschuler commented Dec 16, 2024 •

edited

Loading

jschuler Dec 16, 2024 •

edited

Loading

jschuler Dec 16, 2024

jschuler Dec 16, 2024

jschuler Dec 16, 2024

codecov-commenter commented Dec 16, 2024 •

edited

Loading

yaacov commented Dec 29, 2024

metalice left a comment

metalice Jan 15, 2025

metalice Jan 15, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

metalice Jan 16, 2025

sonarqubecloud bot commented Jan 17, 2025


		import '../create/PlanCreatePage.style.css';

		export const PlanEditPage: React.FC<{

MTV-1726: Edit VMs list in migration plan #1414

Are you sure you want to change the base?

MTV-1726: Edit VMs list in migration plan #1414

Conversation

jschuler commented Dec 16, 2024 • edited Loading

jschuler Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 16, 2024 • edited Loading

Codecov Report

yaacov commented Dec 29, 2024

metalice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed

jschuler commented Dec 16, 2024 •

edited

Loading

jschuler Dec 16, 2024 •

edited

Loading

codecov-commenter commented Dec 16, 2024 •

edited

Loading