Addressed review comments

Author: grevanak-akamai

packages/manager/cypress/e2e/core/linodes/create-linode-with-firewall.spec.ts

@@ -424,7 +424,7 @@ describe('Create Linode with Firewall (Linode Interfaces)', () => {
     // Switch to legacy Config Interfaces
     linodeCreatePage.selectLegacyConfigInterfacesType();
 
-    // Select a firewall for the VPC interface
+    // Select a firewall
     linodeCreatePage.selectFirewall(mockFirewall.label, 'Firewall');
 
     // Confirm Firewall assignment indicator is shown in Linode summary.

packages/manager/cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts

@@ -399,6 +399,12 @@ describe('Community Stackscripts integration tests', () => {
         cy.get('[data-qa-radio]').click({ force: true });
       });
 
+    // Select a firewall
+    linodeCreatePage.selectFirewall(
+      mockFirewall.label,
+      'Public Interface Firewall'
+    );
+
     // Input root password
     // Weak or fair root password cannot rebuild the linode
     cy.get('[id="root-password"]').clear();
@@ -429,11 +435,6 @@ describe('Community Stackscripts integration tests', () => {
     cy.get('[id="root-password"]').type(rootPassword);
     interceptCreateLinode().as('createLinode');
 
-    // Select a firewall
-    linodeCreatePage.selectFirewall(
-      mockFirewall.label,
-      'Public Interface Firewall'
-    );
     ui.button
       .findByTitle('Create Linode')
       .should('be.visible')

packages/manager/src/features/Firewalls/components/FirewallSelect.test.tsx

@@ -123,6 +123,6 @@ describe('FirewallSelect', () => {
       <FirewallSelect value={NO_FIREWALL_ID} />
     );
 
-    expect(await findByDisplayValue(NO_FIREWALL_LABEL)).toBeInTheDocument();
+    await findByDisplayValue(NO_FIREWALL_LABEL);
   });
 });

packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.test.ts

@@ -50,7 +50,6 @@ describe('getLinodeInterfacesPayload', () => {
           vpc_id: 1,
         },
       }),
-      firewall_id: null,
       purpose: 'public' as const,
     };
 
@@ -67,7 +66,6 @@ describe('getLinodeInterfacePayload', () => {
     const networkInterface = {
       ...linodeInterfaceFactoryPublic.build(),
       purpose: 'public' as const,
-      firewall_id: null,
     };
 
     expect(getLinodeInterfacePayload(networkInterface)).toEqual({
@@ -80,7 +78,6 @@ describe('getLinodeInterfacePayload', () => {
     const networkInterface = {
       ...vpcInterface,
       purpose: 'vpc' as const,
-      firewall_id: null,
     };
 
     const newInterface = {
@@ -89,7 +86,6 @@ describe('getLinodeInterfacePayload', () => {
     };
     expect(getLinodeInterfacePayload(networkInterface)).toEqual({
       ...newInterface,
-      firewall_id: null,
     });
   });
 });

packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.ts

@@ -19,9 +19,7 @@ interface VPC extends NonNullable<CreateLinodeInterfacePayload['vpc']> {
 /**
  * We extend the new `CreateLinodeInterfacePayload` to add extra state we need to track
  */
-export interface LinodeCreateInterface
-  extends Omit<CreateLinodeInterfacePayload, 'firewall_id'> {
-  firewall_id: null | number;
+export interface LinodeCreateInterface extends CreateLinodeInterfacePayload {
   purpose: InterfacePurpose;
   vpc: null | VPC;
 }
@@ -54,19 +52,13 @@ export const getCleanedLinodeInterfaceValues = (
  */
 export const getLinodeInterfacePayload = (
   networkInterface: LinodeCreateInterface
-): Omit<CreateLinodeInterfacePayload, 'firewall_id'> & {
-  firewall_id: null | number;
-} => {
+): CreateLinodeInterfacePayload => {
   // ensure only one interface type is present
   const cleanedValues = getCleanedLinodeInterfaceValues(networkInterface);
 
   if (cleanedValues.vpc) {
     const vpcValues = omitProps(cleanedValues.vpc, ['vpc_id']);
-    return {
-      ...omitProps(cleanedValues, ['purpose']),
-      firewall_id: cleanedValues.firewall_id,
-      vpc: vpcValues,
-    };
+    return { ...omitProps(cleanedValues, ['purpose']), vpc: vpcValues };
   }
 
   // The API errors saying address is invalid if we pass an empty string.
@@ -76,10 +68,7 @@ export const getLinodeInterfacePayload = (
     cleanedValues.vlan.ipam_address = null;
   }
 
-  return {
-    ...omitProps(cleanedValues, ['purpose']),
-    firewall_id: cleanedValues.firewall_id,
-  };
+  return omitProps(cleanedValues, ['purpose']);
 };
 
 /**

packages/manager/src/features/Linodes/LinodeCreate/schemas.ts

@@ -7,6 +7,7 @@ import { array, boolean, number, object, string } from 'yup';
 import { CreateLinodeInterfaceFormSchema } from '../LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/utilities';
 
 import type { LinodeCreateFormValues } from './utilities';
+import type { InterfaceGenerationType } from '@linode/api-v4/lib/linodes/types';
 import type { ObjectSchema } from 'yup';
 
 /**
@@ -17,6 +18,12 @@ import type { ObjectSchema } from 'yup';
 export const CreateLinodeSchema: ObjectSchema<LinodeCreateFormValues> =
   BaseCreateLinodeSchema.concat(
     object({
+      firewall_id: number().when('interface_generation', {
+        is: (value: InterfaceGenerationType) => value === 'legacy_config',
+        then: (schema) =>
+          schema.required('Select an option or create a new Firewall.'),
+        otherwise: (schema) => schema.nullable().notRequired(),
+      }),
       firewallOverride: boolean(),
       hasSignedEUAgreement: boolean(),
       interfaces: array(ConfigProfileInterfaceSchema).required(),

packages/manager/src/features/Linodes/LinodeCreate/utilities.test.tsx

@@ -1,4 +1,11 @@
-import { createLinodeRequestFactory } from '@linode/utilities';
+import {
+  createLinodeRequestFactory,
+  getIsLegacyInterfaceArray,
+  linodeConfigInterfaceFactory,
+  linodeInterfaceFactoryPublic,
+  linodeInterfaceFactoryVlan,
+  linodeInterfaceFactoryVPC,
+} from '@linode/utilities';
 
 import {
   getDefaultInterfaceGenerationFromAccountSetting,
@@ -357,6 +364,26 @@ describe('getInterfacesPayload', () => {
   });
 });
 
+describe('getIsLegacyInterfaceArray', () => {
+  it('determines the given interfaces are legacy', () => {
+    const legacyInterfaces = linodeConfigInterfaceFactory.buildList(3);
+    expect(getIsLegacyInterfaceArray(legacyInterfaces)).toBe(true);
+    expect(getIsLegacyInterfaceArray(undefined)).toBe(true);
+    expect(getIsLegacyInterfaceArray([])).toBe(true);
+  });
+
+  it('returns false if the given interfaces are new Linode Interfaces', () => {
+    const linodeInterfacesVlan = linodeInterfaceFactoryVlan.buildList(3);
+    expect(getIsLegacyInterfaceArray(linodeInterfacesVlan)).toBe(false);
+
+    const linodeInterfacesVPC = linodeInterfaceFactoryVPC.buildList(3);
+    expect(getIsLegacyInterfaceArray(linodeInterfacesVPC)).toBe(false);
+
+    const linodeInterfacesPublic = linodeInterfaceFactoryPublic.buildList(3);
+    expect(getIsLegacyInterfaceArray(linodeInterfacesPublic)).toBe(false);
+  });
+});
+
 describe('getLinodeLabelFromLabelParts', () => {
   it('should join items', () => {
     expect(getLinodeLabelFromLabelParts(['my-linode', 'us-east'])).toBe(

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

@@ -25,7 +25,6 @@ import { getDefaultUDFData } from './Tabs/StackScripts/UserDefinedFields/utiliti
 import type { LinodeCreateInterface } from './Networking/utilities';
 import type {
   AccountSettings,
-  CreateLinodeInterfacePayload,
   CreateLinodeRequest,
   FirewallSettings,
   InterfaceGenerationType,
@@ -98,8 +97,8 @@ export const getLinodeCreatePayload = (
     if (shouldUseNewInterfaces) {
       values.interfaces = formValues.linodeInterfaces.map(
         getLinodeInterfacePayload
-      ) as CreateLinodeInterfacePayload[];
-      values.firewall_id = -1;
+      );
+      values.firewall_id = undefined;
     } else {
       values.interfaces = formValues.backup_id
         ? undefined

packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/AddInterfaceForm.tsx

@@ -23,7 +23,6 @@ import { VLANInterface } from './VLAN/VLANInterface';
 import { VPCInterface } from './VPC/VPCInterface';
 
 import type { CreateInterfaceFormValues } from './utilities';
-import type { CreateLinodeInterfacePayload } from '@linode/api-v4';
 
 interface Props {
   linodeId: number;
@@ -64,7 +63,10 @@ export const AddInterfaceForm = (props: Props) => {
       const { errors, values } = await yupResolver(
         CreateLinodeInterfaceFormSchema
       )(
-        valuesWithOnlySelectedInterface as CreateInterfaceFormValues,
+        {
+          ...valuesWithOnlySelectedInterface,
+          firewall_id: valuesWithOnlySelectedInterface.firewall_id!,
+        },
         context,
         options
       );
@@ -79,10 +81,7 @@ export const AddInterfaceForm = (props: Props) => {
 
   const onSubmit = async (values: CreateInterfaceFormValues) => {
     try {
-      const payload = getLinodeInterfacePayload(
-        values
-      ) as CreateLinodeInterfacePayload;
-      await mutateAsync(payload);
+      await mutateAsync(getLinodeInterfacePayload(values));
 
       enqueueSnackbar('Successfully added network interface.', {
         variant: 'success',

packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/utilities.ts

@@ -9,6 +9,9 @@ import type { InferType } from 'yup';
 export const CreateLinodeInterfaceFormSchema =
   CreateLinodeInterfaceSchema.concat(
     object({
+      firewall_id: number().required(
+        'Select an option or create a new Firewall.'
+      ),
       purpose: string()
         .oneOf(['vpc', 'vlan', 'public'])
         .required('You must selected an Interface type.'),