upcoming: [M3-8279] - Conditionally disable regions based on the selected image #10607

bnussman-akamai · 2024-06-24T15:22:40Z

Description 📝

This is for Image Service Gen2 💾
Same changes were made to both Linode Create v1 and v2 flows in this PR
On the Linode Create flow, certain regions will be disabled depending on the Image's capabilities 🌐
- Images with the distributed capability can be deployed to both distributed and core sites
- Images without the distributed capability can only be deployed to core sites
The Region value should be cleared automatically if the user changes the Image and the currently selected region is not compatible
Note that this behavior is somewhat temporary. Once all customers images are migrated to image service gen2, all images will be edge and core compatible, allowing us to remove this code 🗒️

Preview 📷

How to test 🧪

Prerequisites

Turn the MSW on

Verification steps

Warning

Test with both Linode Create and Linode Create v2

Verify that when a distributed compatible image is selected any region can be selected
Verify that when a non-distributed capable image is selected, only core regions can be selected
Verify that if the user changes the Image, the region field is cleared if there is an incompatibility

As an Author I have considered 🤔

github-actions · 2024-06-24T15:49:44Z

Coverage Report: ✅
Base Coverage: 82.39%
Current Coverage: 82.43%

…image on Linode Create

hkhalil-akamai

Verified all functionality in create v1 and v2:

✅ Edge sites are disabled when selecting a core image
✅ Edge sites are enabled when selecting a distributed image
✅ Switching from a distributed image to a core image clears regions field if a core region is selected

Approved pending some minor feedback and one failing unit test. (CI test failures appear to be flake).

hkhalil-akamai · 2024-06-24T21:43:30Z

packages/manager/src/components/ImageSelect/ImageSelect.tsx

+    selection: string | undefined,
+    image: Image | undefined


selection is just the image label right? If so, can we just provide the image?

Selection is the image id.

I chose to just pass the whole image as the second parameter to minimize having to touch tons of other files.

ImageSelect should be removed soon in favor of ImageSelect2, which addresses this, so I think this is okay for now.

Let's keep in mind that <ImageSelect /> won't be deprecated by switching over to the Linode Create v2 flow alone (it's also used in Rebuild flows) so we may live with this for a while. We should at least have a code comment about this here and below for the non-null assertion

packages/manager/src/components/ImageSelect/ImageSelect.tsx

hkhalil-akamai · 2024-06-24T21:44:48Z

packages/manager/src/components/RegionSelect/RegionSelect.utils.tsx

+  const supportedDistributedRegionTypes = [
    'Distributions',
    'StackScripts',
+    'Images',


Should this be a top-level constant?

I will let @hana-linode handle that with Gecko

packages/manager/src/features/Linodes/LinodeCreatev2/Region.test.tsx

hkhalil-akamai · 2024-06-24T22:01:04Z

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx

+    selectedImageID:
+      this.params.imageID ?? this.params.type !== 'Images'
+        ? DEFAULT_IMAGE
+        : undefined,
    // @todo: Abstract and test. UPDATE 5/21/20: lol what does this mean. UPDATE 3/16/23 lol what


Can't wait to switch to create v2 so we can finally remove this comment 😆

abailly-akamai

Logic looks good and confirmed the region select is behaving properly according to the image selection ✅

abailly-akamai · 2024-06-25T13:01:28Z

packages/manager/src/components/ImageSelect/ImageSelect.tsx

+    selection: string | undefined,
+    image: Image | undefined


Let's keep in mind that <ImageSelect /> won't be deprecated by switching over to the Linode Create v2 flow alone (it's also used in Rebuild flows) so we may live with this for a while. We should at least have a code comment about this here and below for the non-null assertion

abailly-akamai · 2024-06-25T13:11:19Z

packages/manager/src/components/SelectRegionPanel/SelectRegionPanel.tsx

+      return disabledRegions;
+    },
+    {}
+  );


I am starting to get worried about seeing a lot of of various logic bits for checking against distributed regions. Not something this PR should necessarily address but it would be nice to start seeing those checks in utils and unit tested in isolation (glad to see it's still tested below! tho there seems to be a failure). This same duplicated is Region.tsx which I assume is intentional?

CC @hana-linode

abailly-akamai · 2024-06-25T13:17:46Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Images.tsx

+    if (
+      image &&
+      !image.capabilities.includes('distributed-images') &&
+      selectedRegion?.site_type === 'distributed'


similar comment here - tho this one isn't unit tested

packages/manager/src/components/SelectRegionPanel/SelectRegionPanel.tsx

@abailly-akamai

…e v1 @abailly-akamai

…ed-on-the-selected-image

mjac0bs

🚢 I just pulled latest and confirmed:

Create Flow v1:

When a distributed compatible image is selected any region can be selected
When a non-distributed capable image is selected, only core regions can be selected
If the user changes the Image, the region field is cleared if there is an incompatibility

Create Flow v2:

When a distributed compatible image is selected any region can be selected
When a non-distributed capable image is selected, only core regions can be selected
If the user changes the Image, the region field is cleared if there is an incompatibility

bnussman-akamai added the Image Service Gen2 label Jun 24, 2024

bnussman-akamai self-assigned this Jun 24, 2024

bnussman-akamai requested a review from a team as a code owner June 24, 2024 15:22

bnussman-akamai requested review from mjac0bs and abailly-akamai and removed request for a team June 24, 2024 15:22

bnussman-akamai force-pushed the M3-8279-conditionally-disable-regions-based-on-the-selected-image branch from b49b73d to 0aeb0bd Compare June 24, 2024 15:24

initial work to disable regions based on image

68bfd23

bnussman-akamai force-pushed the M3-8279-conditionally-disable-regions-based-on-the-selected-image branch from 0aeb0bd to 68bfd23 Compare June 24, 2024 15:28

bnussman-akamai added the Work in Progress label Jun 24, 2024

add unit test

49cd446

bnussman-akamai requested a review from hkhalil-akamai June 24, 2024 16:39

Added changeset: Conditionally disable regions based on the selected …

9667bf5

…image on Linode Create

bnussman-akamai added Work in Progress and removed Work in Progress labels Jun 24, 2024

bnussman added 3 commits June 24, 2024 14:31

adjust based on incorrect ux mocks

f8513fb

update copy

7a33d0b

comments

86bd6bb

bnussman-akamai removed the Work in Progress label Jun 24, 2024

fix linode create v1 bug where image defaults to debian 11 on images tab

6d54c9b

bnussman-akamai added Ready for Review labels Jun 24, 2024

hkhalil-akamai approved these changes Jun 24, 2024

View reviewed changes

feedback @hkhalil-akamai

619d735

abailly-akamai approved these changes Jun 25, 2024

View reviewed changes

bnussman added 3 commits June 25, 2024 09:52

change copy to a distributed region, add unit test for Linode Creat…

246a4c1

…e v1 @abailly-akamai

use shared util for both flows and unit test new util @abailly-akamai

63a71a5

Merge branch 'develop' into M3-8279-conditionally-disable-regions-bas…

066f418

…ed-on-the-selected-image

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Jun 25, 2024

fix typo

dc03a46

mjac0bs approved these changes Jun 25, 2024

View reviewed changes

bnussman-akamai merged commit 449c20f into linode:develop Jun 25, 2024
18 checks passed

This was referenced Jul 8, 2024

Release v1.123.0 - release → staging #10648

Merged

Release v1.123.0 - staging → master #10652

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-8279] - Conditionally disable regions based on the selected image #10607

upcoming: [M3-8279] - Conditionally disable regions based on the selected image #10607

bnussman-akamai commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024 •

edited

Loading

hkhalil-akamai left a comment

hkhalil-akamai Jun 24, 2024

bnussman-akamai Jun 24, 2024 •

edited

Loading

abailly-akamai Jun 25, 2024

hkhalil-akamai Jun 24, 2024

bnussman-akamai Jun 24, 2024

hkhalil-akamai Jun 24, 2024

abailly-akamai left a comment

abailly-akamai Jun 25, 2024

abailly-akamai Jun 25, 2024

abailly-akamai Jun 25, 2024

mjac0bs left a comment

upcoming: [M3-8279] - Conditionally disable regions based on the selected image #10607

upcoming: [M3-8279] - Conditionally disable regions based on the selected image #10607

Conversation

bnussman-akamai commented Jun 24, 2024 • edited Loading

Description 📝

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

github-actions bot commented Jun 24, 2024 • edited Loading

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai Jun 24, 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

Choose a reason for hiding this comment

abailly-akamai 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

mjac0bs left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024 •

edited

Loading

bnussman-akamai Jun 24, 2024 •

edited

Loading