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

feat: [M3-6724]: Linode Config & Interface endpoints, validation, and React Query queries #9418

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Jul 18, 2023

Description 📝

Add and modify existing Linode Config & Interface endpoints, validation, and RQ queries.

Major Changes 🔄

  • New Linode Configs API methods added
  • New Linode Configs types added, some existing types modified
  • Linode Configs RQ queries added
  • Changes in linodes.schema.ts, modification to linodeInterfaceSchema
  • Adjustments & fixes in vpcs.schema.ts

How to test 🧪

As in the previous couple of PRs, there is some temporary code in App.tsx to work with a RQ query.

This PR significantly modifies the long-existing linodeInterfaceSchema, as well as newer VPC-related schemas added in previous PRs.

If you'd like to test those schemas thoroughly against the API spec, the best way would be by creating a new local project and pulling in this branch's api-v4 and validation packages as dependencies. Your package.json file in your project would look something like:

Screenshot 2023-07-22 at 12 33 40 AM

From there you'd have a file like file.js, import the schemas you want to test, and create mock payloads. As an example:

import { linodeInterfaceSchema } from "@linode/validation";

const interfacePayload = {
  purpose: "vpc",
  primary: true,
  subnet: 15,
}; 

try {
  console.log(linodeInterfaceSchema.validateSync([interfacePayload]));
  linodeInterfaceSchema.validateSync([interfacePayload]);
} catch (err) {
  console.log(err);
}

And then run a node command like node .src/file.js to execute the code.

Beyond this, please confirm that you can still create Linodes in Cloud -- there were modifications made to linodeInterfaceSchema so that impacts the Linode creation payload.

@dwiley-akamai dwiley-akamai added Work in Progress @linode/api-v4 Changes are made to the @linode/api-v4 package @linode/validation Changes are made to the @linode/validation package VPC Relating to VPC project labels Jul 18, 2023
@dwiley-akamai dwiley-akamai self-assigned this Jul 18, 2023
)}/configs/${encodeURIComponent(configId)}/interfaces`
),
setMethod('GET')
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ever going to get sorted and/or paginated? If so you need params and x-filter. I think x-filter should be the minimum if we're going to present this data in a table which I assume we wil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a non-paginated finite list returned in devnum order. I asked the API team about params and filters on Wednesday morning; there are no plans to add any at the moment, and based on our UI wireframes, we don't have a real need for them as there won't be a new table for this specific data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary code to facilitate testing. Will be removed prior to merging.

const IP_EITHER_BOTH_NOT_NEITHER =
'A subnet must have either IPv4 or IPv6, or both, but not neither.';

export const vpcsValidateIP = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Across the Linodes, Firewalls, and Databases areas of the codebase, we already have several IP-related validation functions, but they all have slightly different quirks. I think a more thorough-going look at all of those to see if they can be consolidated should be done at some point.

In the meantime I'm trying to avoid adding several more IP-related validation functions. There's a good argument this function is over-engineered, but it captures what we need for these VPC-related validations without significantly growing the number of IP-related validation functions in the codebase. That being said, if there's consensus around breaking this function up, we can give that concern more weight than the concern re: the number of IP-related validation functions.

[
['ipv6', 'ipv4'],
['ipv4', 'ipv6'],
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The array prevents the cyclical dependency error.

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Jul 27, 2023
Comment on lines +166 to +170
primary?: boolean;
subnet?: number | null;
ipv4?: ConfigInterfaceIPv4;
ipv6?: ConfigInterfaceIPv6;
ip_ranges?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a new LinodeConfigInterface that extends Interface instead of having all these optional props?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on how Interface is used across the codebase, when going through the files this seems clearer to me

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jul 28, 2023
value?: string | null,
shouldHaveIPMask?: boolean,
mustBeIPMask?: boolean
): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This may be a personal preference, but updating the method to accept an object for parameters will not only make it more flexible/extensible, but also help others understand what these arguments are doing. The invocation vpcsValidateIP(value, true, false) kinda speaks for itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DevDW I'm going to merge this so work can continue, but comments remain!

@jaalah-akamai jaalah-akamai merged commit ce408e2 into linode:develop Aug 1, 2023
11 checks passed
jaalah-akamai pushed a commit that referenced this pull request Aug 7, 2023
… React Query queries (#9418)

Co-authored-by: Dajahi Wiley <dwiley@linode.com>
jaalah-akamai pushed a commit that referenced this pull request Aug 7, 2023
… React Query queries (#9418)

Co-authored-by: Dajahi Wiley <dwiley@linode.com>
jaalah-akamai pushed a commit that referenced this pull request Aug 8, 2023
… React Query queries (#9418)

Co-authored-by: Dajahi Wiley <dwiley@linode.com>
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! @linode/api-v4 Changes are made to the @linode/api-v4 package @linode/validation Changes are made to the @linode/validation package VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants