upcoming: [M3-8028]: Linode Create Refactor - User Defined Fields #10395

bnussman-akamai · 2024-04-22T19:03:56Z

Description 📝

Adds initial User Defined Field support to the Linode Create flow when deploying from a StackScript

Changes 🔄

Extracts out some until functions from the existing create flow and unit tests them (no changes in logic, only naming changes) 🧪
Adds initial User Defined Field support to the new Linode Create flow

Preview 📷

How to test 🧪

Prerequisites

Check out this PR or use an internal preview link
Enable the Linode Create v2 feature flag

Verification steps

Select various community StackScripts and check the functionality of the user defined fields section
Compare behavior to production

As an Author I have considered 🤔

github-actions · 2024-04-22T21:27:21Z

Coverage Report: ✅
Base Coverage: 81.86%
Current Coverage: 81.86%

abailly-akamai

I got a 500 trying to submit this one ☝️
Tried in prod with exact same specs and it went through

{
    "authorized_users": [],
    "backups_enabled": false,
    "booted": true,
    "image": "linode/almalinux8",
    "label": "setup-ipsec-vpn-us-east",
    "private_ip": false,
    "region": "us-east",
    "root_pass": "[pass]",
    "stackscript_data": {
        "VPN_PASSWORD": "dfasdf4534",
        "VPN_USER": "dddesdfgd",
        "VPN_IPSEC_PSK": "dfgsdfg564"
    },
    "stackscript_id": 37239,
    "tags": [],
    "type": "g6-nanode-1"
}

one thing i noticed is the stackscript_id seems to be missing in the v2 flow but unsure if that's the root cause

abailly-akamai · 2024-04-23T17:05:30Z

...eatures/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/UserDefinedFieldInput.tsx

+
+  if (getIsUDFMultiSelect(userDefinedField)) {
+    const options = userDefinedField
+      .manyof!.split(',')


Can have have better type safety around manyof & oneof (or safer conditionals)?

What approach would you recommend here? Changing the condition to if (udf.manyof)? I can't think of a way to make getIsUDFMultiSelect more typesafe without writing lots more typescript types.

Changing the condition to if (udf.manyof)

yeah i don't have a better solution. maybe not crucial cause we know, but always in favor of being extra verbose to avoid non-null assertions

You can probably accomplish what you're looking for using a nifty feature of TypeScript: type predicates.

I was thinking about that, but wouldn't that require a new type?

interface UserDefinedFieldMulti extends UserDefinedField { manyof: string; } /** * @returns true if a user defined field should be treated as a multi-select */ export const getIsUDFMultiSelect = (udf: UserDefinedField): udf is UserDefinedFieldMulti => { return !!udf.manyof; }

Is there a way to do it cleaner? I'm not sure having a bunch of extra types is worth it

bnussman-akamai · 2024-04-23T17:42:46Z

@abailly-akamai I wasn't able to replicate the 500 error, but I'm assuming the empty placement_group object was confusing the API. I pushed d25d2ca based on that assumption

abailly-akamai

Tested well on my end with various payloads and stackscripts. ✅

Thanks for the extra care of unit testing thoroughly ❤️

dwiley-akamai

Should we expect full parity at this stage when it comes to the images that are available for each StackScript? I noticed the below example:

Screenshots

This branch	Prod

packages/manager/src/features/Linodes/LinodeCreatev2/utilities.ts

...manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/utilities.ts

dwiley-akamai · 2024-04-23T20:43:16Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/StackScriptSelection.tsx

+      {formState.errors.stackscript_id && (
+        <Notice
+          spacingBottom={0}
+          spacingTop={8}
+          text={formState.errors.stackscript_id.message}
+          variant="error"
+        />
+      )}


out of curiosity, how can we hit this error? You have to select a StackScript in order to select an image, and submitting the form with neither just gets you the Image is required. error

I was able to trigger formState.errors.stackscript_id by sending a UDF that was super long

dwiley-akamai · 2024-04-23T20:53:30Z

...eatures/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/UserDefinedFieldInput.tsx

+      .oneof!.split(',')
+      .map((option) => ({ label: option }));
+
+    if (options.length > 4) {


Should the condition be >= here? Storybook says we use radio buttons if there are 3 or fewer options

Going to retain > 4 because that's what the existing create flow uses (see packages/manager/src/features/StackScripts/UserDefinedFieldsPanel/FieldTypes/UserDefinedSelect.tsx)

bnussman-akamai · 2024-04-23T21:41:10Z

@dwiley-akamai There should be full parity with StackScript UDF fields. Your screenshot shows two different StackScripts selected, so I'm not sure if you were referring to UDFs. The icon missing in the new ImageSelect is expected. That hasn't been implemented yet

dwiley-akamai · 2024-04-23T21:47:09Z

@dwiley-akamai There should be full parity with StackScript UDF fields. Your screenshot shows two different StackScripts selected, so I'm not sure if you were referring to UDFs. The icon missing in the new ImageSelect is expected. That hasn't been implemented yet

Whoops, you can disregard that comment. I definitely honed in on "Shadowsocks" in the StackScript name and overlooked that the two StackScripts weren't the same 🤦🏾‍♂️

hkhalil-akamai

Changes look good and I tested deploying some linodes from stackscripts with UDFs. Left some suggestions.

hkhalil-akamai · 2024-04-23T21:46:04Z

...es/manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/StackScriptSelectionRow.tsx

@@ -15,7 +15,7 @@ interface Props {
  disabled?: boolean;
  isSelected: boolean;
  onOpenDetails: () => void;
-  onSelect: () => void;
+  onSelect?: () => void;


Curious as to when this prop would not be passed.

I made the prop optional because in the case of a pre-selected StackScript, the row is disabled and doesn't really need an onClick. I could revert this, but I felt wrong to have a "dummy" onSelect for a row that should never be selected. Happy do go either way on this change

hkhalil-akamai · 2024-04-23T21:53:15Z

...eatures/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/UserDefinedFieldInput.tsx

+
+  if (getIsUDFMultiSelect(userDefinedField)) {
+    const options = userDefinedField
+      .manyof!.split(',')


You can probably accomplish what you're looking for using a nifty feature of TypeScript: type predicates.

...manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/utilities.ts

hkhalil-akamai · 2024-04-23T23:21:13Z

...manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/utilities.ts

+  return udfs.reduce(
+    (accum, udf) => {
+      if (getIsUDFOptional(udf)) {
+        return [[...accum[0]], [...accum[1], udf]];
+      } else {
+        return [[...accum[0], udf], [...accum[1]]];
+      }
+    },
+    [[], []]
+  );


This also might be more tersely expressed as two filters:

Suggested change

return udfs.reduce(

(accum, udf) => {

if (getIsUDFOptional(udf)) {

return [[...accum[0]], [...accum[1], udf]];

} else {

return [[...accum[0], udf], [...accum[1]]];

}

},

[[], []]

);

return [udfs.filter(getIsUDFRequired), udfs.filter(getIsUDFOptional)];

...manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/UserDefinedFields/utilities.ts

hkhalil-akamai

This has been brought up previously but worth mentioning: if we want to match prod behavior with autofilling the Linode label, we would also want to prefill the stackscript name.

dwiley-akamai

Code review ✅
Functionality of UDFs for various StackScripts ✅

bnussman added 3 commits April 22, 2024 11:22

inital work

fa638c0

initial udf support

9e1e27d

clean up and test

a75bd61

bnussman-akamai added the Linode Create Refactor label Apr 22, 2024

bnussman-akamai self-assigned this Apr 22, 2024

bnussman added 3 commits April 22, 2024 15:19

hook up error handling

4d06f8a

control each key indivisually

94fce9b

test more and dial in

eaae07e

bnussman-akamai marked this pull request as ready for review April 22, 2024 20:52

bnussman-akamai requested a review from a team as a code owner April 22, 2024 20:52

bnussman-akamai requested review from dwiley-akamai, carrillo-erik, hkhalil-akamai and abailly-akamai and removed request for a team April 22, 2024 20:52

Added changeset: Linode Create Refactor - Add User Defined Fields

9b51ae9

bnussman-akamai changed the title ~~upcoming: [M3-7872]: Linode Create Refactor - User Defined Fields~~ upcoming: [M3-8028]: Linode Create Refactor - User Defined Fields Apr 23, 2024

abailly-akamai reviewed Apr 23, 2024

View reviewed changes

prevent placement group 500 error

d25d2ca

abailly-akamai approved these changes Apr 23, 2024

View reviewed changes

dwiley-akamai reviewed Apr 23, 2024

View reviewed changes

feedback

0baab0d

bnussman-akamai requested a review from dwiley-akamai April 23, 2024 21:41

bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Apr 23, 2024

hkhalil-akamai approved these changes Apr 23, 2024

View reviewed changes

hkhalil-akamai reviewed Apr 23, 2024

View reviewed changes

bnussman added 2 commits April 23, 2024 20:59

clean up separateUDFsByRequiredStatus

06ea7a3

remove getIsUDFOptional in favor of getIsUDFRequired

e570899

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Apr 24, 2024

fix logic

8040d1c

dwiley-akamai approved these changes Apr 24, 2024

View reviewed changes

carrillo-erik approved these changes Apr 24, 2024

View reviewed changes

bnussman-akamai merged commit 7bf7766 into linode:develop Apr 24, 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-8028]: Linode Create Refactor - User Defined Fields #10395

upcoming: [M3-8028]: Linode Create Refactor - User Defined Fields #10395

bnussman-akamai commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

abailly-akamai left a comment •

edited

abailly-akamai Apr 23, 2024

bnussman-akamai Apr 23, 2024

abailly-akamai Apr 23, 2024

hkhalil-akamai Apr 23, 2024

bnussman-akamai Apr 24, 2024

bnussman-akamai commented Apr 23, 2024

abailly-akamai left a comment

dwiley-akamai left a comment

dwiley-akamai Apr 23, 2024

bnussman-akamai Apr 23, 2024

dwiley-akamai Apr 23, 2024

bnussman-akamai Apr 23, 2024

bnussman-akamai commented Apr 23, 2024

dwiley-akamai commented Apr 23, 2024

hkhalil-akamai left a comment

hkhalil-akamai Apr 23, 2024

bnussman-akamai Apr 24, 2024

hkhalil-akamai Apr 23, 2024

hkhalil-akamai Apr 23, 2024

hkhalil-akamai left a comment

dwiley-akamai left a comment

upcoming: [M3-8028]: Linode Create Refactor - User Defined Fields #10395

upcoming: [M3-8028]: Linode Create Refactor - User Defined Fields #10395

Conversation

bnussman-akamai commented Apr 22, 2024 • edited

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

github-actions bot commented Apr 22, 2024 • edited

abailly-akamai left a comment • 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

Choose a reason for hiding this comment

bnussman-akamai commented Apr 23, 2024

abailly-akamai left a comment

Choose a reason for hiding this comment

dwiley-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

Choose a reason for hiding this comment

bnussman-akamai commented Apr 23, 2024

dwiley-akamai commented Apr 23, 2024

hkhalil-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

Choose a reason for hiding this comment

hkhalil-akamai left a comment

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024 •

edited

abailly-akamai left a comment •

edited