Skip to content

Commit 5f45f6d

Browse files
avivturcursoragent
avivtur
and
cursoragent
authored
Resolves: MTV-5248 | Un-hide EC2 network/storage mapping steps in plan wizard (#2395)
* Un-hide EC2 network/storage mapping steps in plan wizard MTV-5227 hid the mapping steps for EC2 providers, but the backend validates that EC2 plans have proper network and storage mappings. Plans created via UI got empty spec.map and failed with CannotStart due to VMNetworksNotMapped and VMStorageNotMapped conditions. - Un-hide NetworkMap and StorageMap wizard steps for EC2 - Show mapping sections in the review step - Add EC2 to the storage inventory subPath map - Define shared Ec2Network, Ec2Storage, and Ec2VmObject types - Extract subnet IDs from EC2 VMs for network mapping - Treat all EBS volume types as used for storage mapping - Use name-based source for EC2 storage mappings (backend matches Source.Name against volume type) - Add EC2 case to getMapResourceLabel - Remove stale EC2 skip comment from createNetworkMap Resolves: MTV-5248 Signed-off-by: Aviv Turgeman <aturgema@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Address CodeRabbit review comments - Replace unsafe type assertions with type guards (isEc2Vm) - Fix subnet fallback when NetworkInterfaces exists but yields no SubnetId entries - Fix EC2 storage name fallback to prevent dropping mappings when source.name is missing Resolves: MTV-5248 Signed-off-by: Aviv Turgeman <aturgema@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Extract shared EC2 helpers and add unit tests - Move isEc2Vm, Ec2VmLike, getEc2SubnetIds to shared src/utils/types/ec2Inventory.ts (removes duplication) - Add 22 unit tests covering: isEc2Vm type guard, getEc2SubnetIds fallback logic, EC2 buildStorageMappings name-based source, getStorageMappingValues for EC2, and getMapResourceLabel EC2 case Resolves: MTV-5248 Signed-off-by: Aviv Turgeman <aturgema@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Fix TS errors in getMapResourceLabel test Add missing revision and selfLink fields to Ec2Network test fixtures to match the full type shape. Resolves: MTV-5248 Signed-off-by: Aviv Turgeman <aturgema@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Resolves: MTV-5248 | update gitignore for playwright MCP generated files Signed-off-by: Aviv Turgeman <aturgema@redhat.com> --------- Signed-off-by: Aviv Turgeman <aturgema@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 44bd9e3 commit 5f45f6d

17 files changed

Lines changed: 338 additions & 39 deletions

File tree

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,6 @@ testing/locales/
4040
.cursor/skills/personal-*/
4141
.cursor/hooks/personal-*
4242
.cursor/hooks.json
43+
44+
# Playwright MCP artifacts (browser snapshots, console logs, screenshots)
45+
.playwright-mcp/

‎src/plans/create/CreatePlanWizardInner.tsx‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { type FC, useState } from 'react';
33
import type { V1beta1Provider } from '@forklift-ui/types';
44
import { Wizard, WizardStep } from '@patternfly/react-core';
55
import { useForkliftTranslation } from '@utils/i18n';
6-
import { isProviderEc2 } from '@utils/resources';
76

87
import { useStepNavigation } from './hooks/useStepNavigation';
98
import CustomScriptsStep from './steps/customization-scripts/CustomScriptsStep';
@@ -40,11 +39,6 @@ const CreatePlanWizardInner: FC<CreatePlanWizardInnerProps> = ({
4039
const { currentStep, handleStepChange, hasStepErrors } = useStepNavigation();
4140

4241
const hasCreatePlanError = Boolean(createPlanError?.message);
43-
const isEc2 = isProviderEc2(sourceProvider);
44-
45-
const skippedStepIds = new Set<PlanWizardStepId>(
46-
isEc2 ? [PlanWizardStepId.NetworkMap, PlanWizardStepId.StorageMap] : [],
47-
);
4842

4943
const handleSubmit = async () => {
5044
setCreatePlanError(undefined);
@@ -61,7 +55,7 @@ const CreatePlanWizardInner: FC<CreatePlanWizardInnerProps> = ({
6155
isSubmitting ||
6256
hasCreatePlanError ||
6357
(planStepOrder[id] > planStepOrder[currentStep.id as PlanWizardStepId] &&
64-
hasPreviousStepErrors(id, hasStepErrors, skippedStepIds)),
58+
hasPreviousStepErrors(id, hasStepErrors)),
6559
name: planStepNames[id],
6660
});
6761

@@ -91,14 +85,12 @@ const CreatePlanWizardInner: FC<CreatePlanWizardInnerProps> = ({
9185
<WizardStep
9286
key={PlanWizardStepId.NetworkMap}
9387
{...getStepProps(PlanWizardStepId.NetworkMap)}
94-
isHidden={isEc2}
9588
>
9689
<NetworkMapStep />
9790
</WizardStep>,
9891
<WizardStep
9992
key={PlanWizardStepId.StorageMap}
10093
{...getStepProps(PlanWizardStepId.StorageMap)}
101-
isHidden={isEc2}
10294
>
10395
<StorageMapStep />
10496
</WizardStep>,

‎src/plans/create/steps/__tests__/getMapResourceLabel.ec2.test.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import type { InventoryStorage } from 'src/utils/hooks/useStorages';
2+
3+
import { describe, expect, it } from '@jest/globals';
4+
5+
import type { ProviderNetwork } from '../../types';
6+
import { getMapResourceLabel } from '../utils';
7+
8+
describe('getMapResourceLabel - EC2', () => {
9+
it('returns name for EC2 network', () => {
10+
const network: ProviderNetwork = {
11+
id: 'subnet-abc123',
12+
name: 'my-subnet',
13+
providerType: 'ec2',
14+
revision: 1,
15+
selfLink: '/providers/ec2/uid/networks/subnet-abc123',
16+
};
17+
expect(getMapResourceLabel(network)).toBe('my-subnet');
18+
});
19+
20+
it('returns name for EC2 storage (volume type)', () => {
21+
const storage: InventoryStorage = {
22+
id: 'gp3',
23+
name: 'gp3',
24+
providerType: 'ec2',
25+
revision: 1,
26+
selfLink: '/providers/ec2/uid/storages/gp3',
27+
};
28+
expect(getMapResourceLabel(storage)).toBe('gp3');
29+
});
30+
31+
it('returns empty string for EC2 resource with no name', () => {
32+
const network: ProviderNetwork = {
33+
id: 'subnet-abc123',
34+
name: '',
35+
providerType: 'ec2',
36+
revision: 1,
37+
selfLink: '',
38+
};
39+
expect(getMapResourceLabel(network)).toBe('');
40+
});
41+
42+
it('returns empty string for undefined resource', () => {
43+
expect(getMapResourceLabel(undefined)).toBe('');
44+
});
45+
});

‎src/plans/create/steps/network-map/utils.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import type {
99
import { DEFAULT_NETWORK, Namespace } from '@utils/constants';
1010
import { isEmpty } from '@utils/helpers';
1111
import { t } from '@utils/i18n';
12+
import { getEc2SubnetIds, isEc2Vm } from '@utils/types/ec2Inventory';
1213

1314
import type { CategorizedSourceMappings, MappingValue, ProviderNetwork } from '../../types';
1415
import { hasMultiplePodNetworkMappings } from '../../utils/hasMultiplePodNetworkMappings';
@@ -25,7 +26,7 @@ type ValidateNetworkMapParams = {
2526
};
2627

2728
const toNetworksOrProfiles = (vm: ProviderVirtualMachine): string[] => {
28-
if (vm.providerType === (PROVIDER_TYPES.ec2 as string)) return [];
29+
if (isEc2Vm(vm)) return getEc2SubnetIds(vm);
2930

3031
switch (vm.providerType) {
3132
case PROVIDER_TYPES.vsphere: {

‎src/plans/create/steps/review/ReviewStep.tsx‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
11
import type { FC } from 'react';
2-
import { useWatch } from 'react-hook-form';
3-
import { PROVIDER_TYPES } from 'src/providers/utils/constants';
42

53
import WizardStepContainer from '@components/common/WizardStepContainer';
64
import { EmptyState, Spinner } from '@patternfly/react-core';
75
import { useForkliftTranslation } from '@utils/i18n';
86

97
import { planStepNames, PlanWizardStepId } from '../../constants';
108
import { useCreatePlanFormContext } from '../../hooks/useCreatePlanFormContext';
11-
import { GeneralFormFieldId } from '../general-information/constants';
129

1310
import CustomScriptsReviewSection from './CustomScriptsReviewSection';
1411
import GeneralInfoReviewSection from './GeneralInfoReviewSection';
@@ -33,11 +30,8 @@ const ReviewStep: FC<ReviewStepProps> = ({
3330
}) => {
3431
const { t } = useForkliftTranslation();
3532
const {
36-
control,
3733
formState: { isSubmitting },
3834
} = useCreatePlanFormContext();
39-
const sourceProvider = useWatch({ control, name: GeneralFormFieldId.SourceProvider });
40-
const isEc2 = sourceProvider?.spec?.type === PROVIDER_TYPES.ec2;
4135

4236
if (isSubmitting) {
4337
return (
@@ -64,8 +58,8 @@ const ReviewStep: FC<ReviewStepProps> = ({
6458
>
6559
<GeneralInfoReviewSection />
6660
<VirtualMachinesReviewSection />
67-
{!isEc2 && <NetworkMapReviewSection />}
68-
{!isEc2 && <StorageMapReviewSection />}
61+
<NetworkMapReviewSection />
62+
<StorageMapReviewSection />
6963
<MigrationTypeReviewSection isLiveMigrationFeatureEnabled={isLiveMigrationFeatureEnabled} />
7064
<OtherSettingsReviewSection isLiveMigrationFeatureEnabled={isLiveMigrationFeatureEnabled} />
7165
<CustomScriptsReviewSection />

‎src/plans/create/steps/utils.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { PROVIDER_TYPES } from 'src/providers/utils/constants';
12
import type { InventoryStorage } from 'src/utils/hooks/useStorages';
23

34
import type { ProviderNetwork } from '../types';
@@ -17,20 +18,20 @@ export const getMapResourceLabel = (
1718
}
1819

1920
switch (resource.providerType) {
20-
case 'openshift': {
21-
// OpenShift resources have namespace from OpenshiftResource base type
21+
case PROVIDER_TYPES.openshift: {
2222
if (resource.namespace) {
2323
return `${resource.namespace}/${resource.name}`;
2424
}
2525
return resource.name;
2626
}
27-
case 'hyperv':
28-
case 'ova':
29-
case 'vsphere':
30-
case 'openstack': {
27+
case PROVIDER_TYPES.ec2:
28+
case PROVIDER_TYPES.hyperv:
29+
case PROVIDER_TYPES.ova:
30+
case PROVIDER_TYPES.vsphere:
31+
case PROVIDER_TYPES.openstack: {
3132
return resource.name || '';
3233
}
33-
case 'ovirt': {
34+
case PROVIDER_TYPES.ovirt: {
3435
// Use path for oVirt if available, fall back to name for storage resources
3536
if ('path' in resource && resource.path) {
3637
return resource.path;

‎src/plans/create/types.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { TargetPowerState, TargetPowerStateValue } from 'src/plans/constant
33
import type { StorageMapping, TargetStorage } from 'src/storageMaps/utils/types';
44
import type { InventoryNetwork } from 'src/utils/hooks/useNetworks';
55
import type { InventoryStorage } from 'src/utils/hooks/useStorages';
6+
import type { Ec2Network } from 'src/utils/types/ec2Inventory';
67

78
import type {
89
HypervNetwork,
@@ -53,7 +54,8 @@ export type ProviderNetwork =
5354
| OVirtNetwork
5455
| VSphereNetwork
5556
| OvaNetwork
56-
| HypervNetwork;
57+
| HypervNetwork
58+
| Ec2Network;
5759

5860
type VsphereVirtualMachine = VSphereVM & {
5961
changeTrackingEnabled: boolean;

‎src/plans/create/utils/createNetworkMap.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,6 @@ export const createNetworkMap = async ({
105105
namespace: project,
106106
},
107107
spec: {
108-
// EC2 plans skip the mapping step entirely, producing an empty array
109108
map: (mappings ?? []).reduce(
110109
(acc: V1beta1NetworkMapSpecMap[], { sourceNetwork, targetNetwork }) => {
111110
if (sourceNetwork.name) {

‎src/plans/create/utils/getVMNetworksOrProfiles.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ import { DefaultNetworkLabel } from 'src/plans/details/tabs/Mappings/utils/const
22
import { PROVIDER_TYPES } from 'src/providers/utils/constants';
33

44
import type { OVirtNicProfile, ProviderVirtualMachine } from '@forklift-ui/types';
5+
import { getEc2SubnetIds, isEc2Vm } from '@utils/types/ec2Inventory';
56

67
const getNetworksForVM = (vm: ProviderVirtualMachine) => {
7-
if (vm.providerType === (PROVIDER_TYPES.ec2 as string)) return [];
8+
if (isEc2Vm(vm)) return getEc2SubnetIds(vm);
89

910
switch (vm.providerType) {
1011
case PROVIDER_TYPES.vsphere: {

‎src/providers/details/tabs/VirtualMachines/utils/types/Ec2VM.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ export type Ec2InstanceDetails = {
99
PrivateIpAddress?: string;
1010
VpcId?: string;
1111
SubnetId?: string;
12+
NetworkInterfaces?: { SubnetId?: string }[];
13+
BlockDeviceMappings?: { Ebs?: { VolumeType?: string } }[];
1214
};
1315

1416
// EC2 VM as returned by the inventory API (/providers/ec2/{uid}/vms).

0 commit comments

Comments
 (0)