upcoming: [M3-7621] - Add PlacementGroups to Linode Migrate Flow #10339

carrillo-erik · 2024-04-02T11:49:47Z

Description 📝

This PR adds Placement Groups to the Linode migrate flow. This customer journey now includes the ability to assign a Linode to an existing Placement Group during the migration process. Placement Group selection is optional.

Changes 🔄

List any change relevant to the reviewer.

Integrate the PlacementGroupsSelect component into the ConfigureForm component.
Add Placement Group data in the Linode migrate payload.

Target release date 🗓️

04/15/2024

Preview 📷

Before	After

How to test 🧪

Prerequisites

(How to setup test environment)

Have the placement-group customer tag on the user account used for testing.
Point to the dev environment using the Cloud Manager dev tools.
Have at least one Placement Group entity in the London, England, UK region.

Verification steps

(How to verify changes)

Verify the UI updates, which include the addition of the PlacementGroupsSelect component in the Linode migrate flow.
Verify that you can proceed as usual without selecting a Placement Group.

As an Author I have considered 🤔

Check all that apply

github-actions · 2024-04-02T12:14:09Z

Coverage Report: ✅
Base Coverage: 81.79%
Current Coverage: 81.79%

packages/manager/src/features/Linodes/MigrateLinode/MigrateLinode.tsx

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx

…ming/M3-7621

…anager into upcoming/M3-7621

carrillo-erik · 2024-04-11T16:42:38Z

@abailly-akamai I pulled down the changes you made and I'm still experiencing the same behavior where after selecting a new region, the placement group selection from the previous region persists.

abailly-akamai · 2024-04-11T18:24:43Z

@carrillo-erik not sure what you are looking at without posting anything. It's fixed on the migrate modal

Screen.Recording.2024-04-11.at.14.22.51.mov

packages/manager/src/queries/linodes/linodes.ts

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx

packages/manager/src/queries/linodes/linodes.ts

abailly-akamai

Looks good! approving after reviews and helping with the code 👍

abailly-akamai · 2024-04-12T16:57:03Z

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx

    handleSelectRegion,
    helperText,
    linodeType,
    selectedRegion,
  } = props;

  const flags = useFlags();
+  const showPlacementGroups = Boolean(flags.placementGroups?.enabled);


@carrillo-erik quick note that this will have to be updated with the new useIsPlacementGroupsEnabled once merged

see https://github.com/linode/manager/pull/10372/files#diff-5599f7d817d6e26a53d2dcd1ba5a5394eb79fb65203c11dca4d0c7fb7ce459afR131

You can create a follow up ticket as needed, we don't want to forget that one

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.test.tsx

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx

abailly-akamai · 2024-04-15T13:18:51Z

@carrillo-erik in addition to addressing @bnussman-akamai 's comment, I forgot that you also need to invalidate PG data on in the migration hook if a placement group is present in the payload.

take a look at how it's done in the linode create hook

bnussman-akamai · 2024-04-15T13:22:29Z

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx

@@ -154,6 +192,24 @@ export const ConfigureForm = React.memo((props: Props) => {
              {...panelPrice(selectedRegion, selectedRegionPrice, 'new')}
            />
          )}
+          {showPlacementGroups && (
+            <PlacementGroupsSelect


Slightly unrelated, but I do find it possibly concerning that the PlacementGroupSelect does not render if placement groups haven't loaded. It should probably render with a field error.

What is the cause of concern?

@abailly-akamai I think what @bnussman-akamai might be stating is that the PlacementGroupSelect does not render on the initial render of the ConfigureForm component. The placement groups are fetched after a user has selected a region on the RegionSelect component and then PlacementGroupSelect is displayed. However, since this field is optional, I think rendering a field error is not appropriate in this case.

I was trying to test this PR against the production API because I wanted to test how this new functionality behaved with DC specific pricing. I just found it a bit confusing that the Select did not render at all even with the feature flag on.

It won't be a huge issue if we never change it, but I think it would make sense to render it at all times with a loading / error state just for visibility of the feature and consistency with all other entity Selects throughout the codebase

This PR Expected

I don't think we want to do that, and bring more attention to an error that isn't an error (we don't have to select a placement group as it is optional to pick one during the migration process). If anything, we could just show disabled it with "none" and enable it when a selected region with placement group capability is selected. However, UX decided to just hide it for the time being. Perhaps we can revisit for phase 1. @carrillo-erik please add a comment to reflect this decision.

@bnussman-akamai hope this addressed your concerns

I don't think we're completely on the same page. I'm suggesting https://github.com/linode/manager/blob/develop/packages/manager/src/components/PlacementGroupsSelect/PlacementGroupsSelect.tsx#L70-L72 should be removed in case fetching placement groups errors out. While it's unlikely, it could happen at any time to any user.

ahhh thanks for clarifying - will make a separate ticket to handle this

@bnussman-akamai I see your point, however, implementing the change you suggested would also present the user with a loading/spinning icon within the PlacementGroupsSelect component until they interact with the RegionSelect. That could confuse users to think that something in the page has not finished loading. Displaying an error state would also be confusing given that the field is optional as Alban stated.

thanks for pointing this out: M3-7993

Thank you both!

…ming/M3-7621

carrillo-erik · 2024-04-16T15:03:15Z

There exists some use cases where error may arise during the Placement Group data fetching that were not considered during the UI/UX planning phase. Ticket M3-7993 was created to keep track of this remaining issues and revisit the strategy to resolve gracefully from error.

…ode#10339) * upcoming: [M3-7621] - Add PlacementGroups to migrate flow * Add feature flag logic to hide PG in migrate linode flow * Handle PG select reset on region change * Add conditional logic to solve state update problem in pg select * Remove useEffect * Add changeset * Fix failing unit test * improve selection types * Handle helper for select and migration types * Add changeset and fix typo * Add unit test for pg select component * Update unit test wording * Fix typo and invalidate pg data if pg in migration payload --------- Co-authored-by: Alban Bailly <abailly@akamai.com>

upcoming: [M3-7621] - Add PlacementGroups to migrate flow

d0ef5e9

carrillo-erik requested a review from a team as a code owner April 2, 2024 11:49

carrillo-erik requested review from bnussman-akamai, hkhalil-akamai and abailly-akamai and removed request for a team April 2, 2024 11:49

carrillo-erik self-assigned this Apr 2, 2024

carrillo-erik added the Placement Groups label Apr 2, 2024

abailly-akamai reviewed Apr 2, 2024

View reviewed changes

hkhalil-akamai reviewed Apr 2, 2024

View reviewed changes

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx Outdated Show resolved Hide resolved

carrillo-erik and others added 6 commits April 3, 2024 09:30

Add feature flag logic to hide PG in migrate linode flow

eb45e9f

Handle PG select reset on region change

0d35b29

Add conditional logic to solve state update problem in pg select

2c4b5ce

Merge branch 'develop' of https://github.com/linode/manager into upco…

2505d22

…ming/M3-7621

Merge branch 'upcoming/M3-7621' of https://github.com/carrillo-erik/m…

15a68c6

…anager into upcoming/M3-7621

Remove useEffect

541f95a

carrillo-erik requested a review from abailly-akamai April 11, 2024 13:15

carrillo-erik and others added 3 commits April 11, 2024 06:16

Add changeset

703bb50

Fix failing unit test

4f32cd4

improve selection types

4961364

abailly-akamai reviewed Apr 11, 2024

View reviewed changes

packages/manager/src/queries/linodes/linodes.ts Outdated Show resolved Hide resolved

carrillo-erik commented Apr 11, 2024

View reviewed changes

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx Outdated Show resolved Hide resolved

packages/manager/src/queries/linodes/linodes.ts Outdated Show resolved Hide resolved

packages/manager/src/queries/linodes/linodes.ts Outdated Show resolved Hide resolved

abailly-akamai and others added 3 commits April 11, 2024 17:12

Handle helper for select and migration types

c41fb50

Add changeset and fix typo

0cba653

Add unit test for pg select component

f9217c1

carrillo-erik requested review from hkhalil-akamai and abailly-akamai April 12, 2024 13:45

Update unit test wording

b8ef722

abailly-akamai approved these changes Apr 12, 2024

View reviewed changes

carrillo-erik added the Add'tl Approval Needed Waiting on another approval! label Apr 12, 2024

bnussman-akamai reviewed Apr 12, 2024

View reviewed changes

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.test.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Linodes/MigrateLinode/ConfigureForm.tsx Show resolved Hide resolved

bnussman-akamai reviewed Apr 15, 2024

View reviewed changes

carrillo-erik added 2 commits April 15, 2024 13:29

Merge branch 'develop' of https://github.com/linode/manager into upco…

727f186

…ming/M3-7621

Fix typo and invalidate pg data if pg in migration payload

5159bcd

bnussman-akamai approved these changes Apr 16, 2024

View reviewed changes

carrillo-erik merged commit e2f610e into linode:develop Apr 16, 2024
18 checks passed

This was referenced Apr 25, 2024

Release v.118.0 - release → staging #10409

Closed

Release v1.118.0 - release → staging #10410

Merged

Release v1.118.0 - staging → master #10416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-7621] - Add PlacementGroups to Linode Migrate Flow #10339

upcoming: [M3-7621] - Add PlacementGroups to Linode Migrate Flow #10339

carrillo-erik commented Apr 2, 2024 •

edited

github-actions bot commented Apr 2, 2024 •

edited

carrillo-erik commented Apr 11, 2024

abailly-akamai commented Apr 11, 2024

abailly-akamai left a comment

abailly-akamai Apr 12, 2024

abailly-akamai commented Apr 15, 2024

bnussman-akamai Apr 15, 2024

abailly-akamai Apr 15, 2024

carrillo-erik Apr 15, 2024

bnussman-akamai Apr 15, 2024

abailly-akamai Apr 16, 2024

bnussman-akamai Apr 16, 2024 •

edited

abailly-akamai Apr 16, 2024

carrillo-erik Apr 16, 2024

abailly-akamai Apr 16, 2024

bnussman-akamai Apr 16, 2024

carrillo-erik commented Apr 16, 2024

upcoming: [M3-7621] - Add PlacementGroups to Linode Migrate Flow #10339

upcoming: [M3-7621] - Add PlacementGroups to Linode Migrate Flow #10339

Conversation

carrillo-erik commented Apr 2, 2024 • edited

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

github-actions bot commented Apr 2, 2024 • edited

carrillo-erik commented Apr 11, 2024

abailly-akamai commented Apr 11, 2024

abailly-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly-akamai commented Apr 15, 2024

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

bnussman-akamai Apr 16, 2024 • edited

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

carrillo-erik commented Apr 16, 2024

carrillo-erik commented Apr 2, 2024 •

edited

github-actions bot commented Apr 2, 2024 •

edited

bnussman-akamai Apr 16, 2024 •

edited