-
Notifications
You must be signed in to change notification settings - Fork 373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upcoming: [M3-9535] - Support VPC interfaces on updated Linode Create Networking flow #11847
upcoming: [M3-9535] - Support VPC interfaces on updated Linode Create Networking flow #11847
Conversation
const { errors } = await yupResolver( | ||
schema, | ||
{}, | ||
{ mode: 'async', raw: true } | ||
)(transformedValues as LinodeCreateFormValues, context, options); | ||
)(values, context, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more as
type casts 🎉
The type of schema
now actually matches the form's values (LinodeCreateFormValues
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
firewall_id: number().nullable().notRequired(), | ||
placement_group: PlacementGroupPayloadSchema, | ||
placement_group: PlacementGroupPayloadSchema.notRequired().default(undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not intuitive to me, but this is how I had to handle an optional object with required fields.
Let me know if there is a better way to handle this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.notRequired().nullable()
maybe? (haven't tried)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue unfortunately
notRequired
includes nullable already https://github.com/jquense/yup?tab=readme-ov-file#schemanotrequired-schema
Either, way it doesn't work as I expect for optional object with required fields
@@ -527,7 +543,7 @@ export interface CreateLinodeRequest { | |||
* This is used to set the swap disk size for the newly-created Linode. | |||
* @default 512 | |||
*/ | |||
swap_size?: number; | |||
swap_size?: number | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the type-safety is largely improved, I had to make these changes so that this type matches our validation schema
In most cases, if the API allows a value to be omitted, it will also gracefully handle null
and treat it the same.
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving the types here and avoiding casting for such an important flow.
I did not notice anything wrong while testing, and noticed no regressions with existing flow with flag turned off 9In DEVENV). but added some comments that hopefully can improve the PR. Also adding below a few questions:
- PR review: reliance on devenv to review PRs is a big ask, and a not so team friendly approach. Is there anything that can be done by way of mocking to facilitate the review process for this feature?
Some small UI considerations:
VLAN not being available in the region selected should get a much better (and visible) treatment. Maybe closer to the kind of warnings we get in the plan selection
When creating a VOC from the Linode create flow and it gets p[laced in the select, the error should go away
} | ||
} | ||
} | ||
return errors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could come up with a cleaner way, something like:
export const transformLegacyInterfaceErrorsToLinodeInterfaceErrors = (
errors: APIError[]
) => {
return errors.map(error => ({
...error,
field: Object.entries(legacyFieldToNewFieldMap).reduce(
(field, [key, value]) =>
field ? field.replace(key, value) : field,
error.field
).replace('interfaces', 'linodeInterfaces')
}));
};
May not be as readable and not 100% this is the best path, but it avoids mutating which is bad practice.
firewall_id: number().nullable().notRequired(), | ||
placement_group: PlacementGroupPayloadSchema, | ||
placement_group: PlacementGroupPayloadSchema.notRequired().default(undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.notRequired().nullable()
maybe? (haven't tried)
const { errors } = await yupResolver( | ||
schema, | ||
{}, | ||
{ mode: 'async', raw: true } | ||
)(transformedValues as LinodeCreateFormValues, context, options); | ||
)(values, context, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
/** | ||
* A property with a string literal type indicating whether the Linode is encrypted or unencrypted. | ||
* @default 'enabled' (if the region supports LDE) | ||
*/ | ||
disk_encryption?: EncryptionStatus; | ||
disk_encryption?: EncryptionStatus | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but does it open the door to having ALL our APIv4 POST/PUT payload types have to inherit this pattern eventually? Nothing wrong with it, but this is a considerable change to bring to the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think api-v4 and validation should always match what the API supports, which usually means treating null and omitting the value the same.
I'm sure apiv4 is inconstant with this and I'm confident validation is even more inconstant with it
@abailly-akamai Just put up #11875 to hopefully help with some Linode Interface mocking. This doesn't handle [M3-9097] yet but hopefully alleviates some of the burden 🤞 will begin reviewing this PR shortly! |
Cloud Manager UI test results🔺 1 failing test on test run #9 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/linode-config.spec.ts" |
…ork-connection-drawer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Old UI - confirmed creating Linodes with various interfaces including VPC
✅ New UI
- confirmed creating Linodes with legacy interfaces (with various interfaces including VPC)
- Confirmed creating Linodes with new interfaces (including VPC) and attaching firewalls
✅ Confirmed rebuilding flow for each of the cases above ^
❓ On the new UI, using legacy configs, I'm unable to create a Linode with two public interfaces (expected) but I see no UI changes - there's no error/network request:
public-interface-no-error.mov
Good catch. 🙏 I didn't try that case. I surfaced that error in 6d02652 |
…rovide is checked (#11902) Fixes validation bug affecting the Linode Rebuild flow When trying to rebuild a Linode with the Reuse user data previously provided checkbox checked, a client-side validation error would occur Caused by upcoming: [M3-9535] - Support VPC interfaces on updated Linode Create Networking flow #11847 In that PR, I think I correctly aligned our validation schemas with the API's behavior, but I failed to realize that the changes I made to MetadataSchema would break this flow. Co-authored-by: Banks Nussman <banks@nussman.us>
Description 📝
as
casting as much as possibleNote
I put a lot of focus on improving the type-safety of the Linode Create Flow so to keep PRs small. I will follow this PR up with more PRs to finish the VPC section and to implement other changes.
Preview 📷
How to test 🧪
Prerequisites
Linode Interfaces
Cloud Manager feature flag is enabledVerification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅