Skip to content
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

change: [poc] - Better subnet validation behavior on VPCCreate page #9659

Merged
merged 16 commits into from
Sep 13, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Sep 11, 2023

Description 📝

Previously when working on the VPCCreate page, I'd made two validation versions:

  • The first one did not use the CreateVPCSchema validation for subnets (it validated subnets before using the schema) but had the desired UI -- this one got merged in
  • The second one that used the validation schema had UI downsides, where you couldn't leave a subnet blank and have it be ignored

In the current reason, errors for specific subnets might sometimes appear as general subnet errors, because of how subnet validation happened... Wasn't the biggest fan of that behavior, so I looked into this some more. That's where this PR comes in -- it uses the CreateVPCSchema validation and processes any subnet specific errors returned by the API, while still having the same UI behavior of the current version.

Major Changes 🔄

  • The logic for this PR is built off the logic of the pr I linked above - I basically just added a map of the UI subnet indexes to the indexes of the subnets that get sent to the backend 😅
  • removed the now unused subnet validation logic

Preview 📷

Before After
image image

How to test 🧪

yarn test formikErrorUtils.test.ts

  1. Try creating VPCs with subnets and ensure that subnet-specific errors correspond with the correct subnet, and that general subnet errors still get handled the same

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Sep 11, 2023
@coliu-akamai coliu-akamai self-assigned this Sep 11, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review September 11, 2023 13:48
@coliu-akamai coliu-akamai changed the title change: [poc] - Better subnet validation on VPCCreate page change: [poc] - Better subnet validation behavior on VPCCreate page Sep 11, 2023
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this! 🚀 Left some minor comments on a few things.

Regarding general subnet errors: are there any aside from the max subnet limit that would get displayed from what you've seen?

coliu-akamai and others added 5 commits September 13, 2023 13:27
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Sep 13, 2023

@dwiley-akamai I've also seen subnet errors for having 2+ subnets with the same label or same IP range (these come in as subnets.label or subnets.ipv4)

image

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Sep 13, 2023
visualToAPISubnetMapping: {}
) => {
const combinedSubnets: SubnetFieldState[] = [];
for (let i = 0; i < values.subnets.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we can simplify this by using a .map instead. That way we don't have to push the values to an array. We can also destructure the errors in the if block const errorData = errors[apiSubnetIdx];

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because the validation looks good.

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 13, 2023
@coliu-akamai coliu-akamai merged commit c2cf937 into linode:develop Sep 13, 2023
@coliu-akamai coliu-akamai deleted the create-vpc-poc-validation branch December 19, 2023 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants