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: [M3-7227] - Update VPC-related API types in accordance with new API changes #9824

Merged
merged 10 commits into from
Oct 24, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Oct 20, 2023

Description 📝

This PR updates interface and subnet return object in the API package to make sure they match the updated API spec, and fixes any type errors that result from that.

Changes 🔄

List any change relevant to the reviewer.

  • Updated interface and subnet object types
  • Updated subnets and linodeConfigInterface factories
  • Fixed type errors in VPC and Linode related files/tests
  • Update array lengths in subnet related factories due to a a memory issue

Because these changes had to do with the subnet/interface return object, not the create payloads, there shouldn't be any validation package issues.

Preview 📷

There should be no visual changes.

How to test 🧪

Prerequisites

  • Use the dev environment and make sure your account has vpc tags
  • Also test stuff using the MSW to make sure that the mock data doesn't break (updated some factories)

Verification steps

Verify that there are no regressions within the flows that these api changes affect:

  • Subnets Table
  • Assigning linodes to subnets via the drawer
  • Unassigning linodes via the drawer
  • Linode Configs - add/edit configs

Perform a general check of VPC related flows not mentioned above:

  • Create VPC
  • Edit/Delete VPC
  • create subnets in already existing vpc
  • edit/delete subnets
  • Create linode with VPC

Additional tests

Test the following unit tests

  • yarn test VPC
  • yarn test LinodeEntityDetail

Test all VPC integration tests, test config-related integration tests

yarn cy:run -s 'cypress/e2e/core/vpc/vpc-create.spec.ts' 
yarn cy:run -s 'cypress/e2e/core/vpc/vpc-details-page.spec.ts'
yarn cy:run -s 'cypress/e2e/core/vpc/vpc-landing-page.spec.ts'
yarn cy:run -s 'cypress/e2e/core/vpc/vpc-navigation.spec.ts'
yarn cy:run -s 'cypress/e2e/core/linodes/linode-config.spec.ts'

As an Author I have considered 🤔

Check all that apply

  • 👀 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

@coliu-akamai coliu-akamai added @linode/api-v4 Changes are made to the @linode/api-v4 package VPC Relating to VPC project labels Oct 20, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review October 20, 2023 18:39
@coliu-akamai coliu-akamai requested a review from a team as a code owner October 20, 2023 18:39
@coliu-akamai coliu-akamai requested review from jdamore-linode and mjac0bs and removed request for a team October 20, 2023 18:39
@coliu-akamai coliu-akamai self-assigned this Oct 20, 2023
@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Oct 20, 2023

Consistently getting this fatal error for the test manager:
FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory

Will be looking into it (but also am a bit confused!)

@coliu-akamai coliu-akamai changed the title change: [M3-7227] - Update API types in accordance with new API changs change: [M3-7227] - Update VPC-related API types in accordance with new API changes Oct 20, 2023
coliu-akamai and others added 4 commits October 20, 2023 15:58
…56392.md

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…3419.md

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@coliu-akamai coliu-akamai added the Help Wanted Teamwork makes the dream work! label Oct 20, 2023
@coliu-akamai coliu-akamai force-pushed the feat-m3-7227 branch 2 times, most recently from 4828cd9 to 029a75e Compare October 23, 2023 18:08
@coliu-akamai coliu-akamai added Ready for Review and removed Help Wanted Teamwork makes the dream work! labels Oct 23, 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.

Assigning and unassigning Linodes to subnets ✅
Subnets table ✅
Creating and deleting subnets ✅
Adding and editing Linode configs ✅
Creating, editing, and deleting a VPC ✅
Creating a linode with a VPC assigned ✅

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Without seeing any failed API requests or console errors, I was able to...

  • Verify the updates to the API types and factories match the orange highlighted changes in the doc attached to the internal ticket
  • Perform the action items for a subnet in the Subnets Table
  • Assign linodes to subnets via the drawer
  • Unassign linodes via the drawer
  • Linode Configs - add/edit configs

General check of VPC related flows not mentioned above:

  • Create VPC

  • Edit/Delete VPC

  • create subnets in already existing vpc

  • edit/delete subnets

  • Create linode with VPC

  • yarn test VPC (note: there seems to be an unrelated issue with a VPCDetail test regarding VPC description length, looking into it)

    • FWIW, these tests all passed consistently for me and I didn't see any flake.
  • yarn test LinodeEntityDetail

  • All VPC integration tests, test config-related integration tests

    • Dajahi's on top of this one (edit: wait, that was the unit test), but here's what I saw when running these tests locally:
    • Failure on vpc-details-page.spec.ts: `
       can delete a subnet from the VPC details page:
     CypressError: Timed out retrying after 80050ms: `cy.click()` failed because this element:

<li class="MuiButtonBase-root MuiMenuItem-root Mui-disabled MuiMenuItem-gutters Mui-disabled MuiMenuItem-root Mui-disabled MuiMenuItem-gutters css-1h8fx4b-MuiButtonBase-root-MuiMenuItem-root" tabindex="-1" role="menuitem" aria-disabled="true" data-qa-action-menu-item="Delete">...</li>

has CSS pointer-events: none

pointer-events: none` prevents user mouse interaction.

Fix this problem, or use {force: true} to disable error checking.

@coliu-akamai
Copy link
Contributor Author

thanks Mariah, will look into the failing cypress test!
Also removed the comment about the unit test flake - that was related to the heap limit issue

@coliu-akamai
Copy link
Contributor Author

@mjac0bs cypress test should be fixed now -- made sure that the mock subnet in the test has 0 linodes assigned to it from the start.

The issue was due to the change at line 25 of this file, where every subnet created now automatically has 5 linodes assigned to it. We can only delete subnets with 0 assigned linodes. Thanks Dajahi and Mariah for catching -- should have double checked the cypress tests too!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

The issue was due to the change at line 25 of this file, where every subnet created now automatically has 5 linodes assigned to it. We can only delete subnets with 0 assigned linodes

Thanks for the explanation, that makes sense!

Reran the vpc integration tests locally and all are passing.
image

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Oct 23, 2023
@coliu-akamai coliu-akamai merged commit f0b17fe into linode:develop Oct 24, 2023
11 checks passed
@coliu-akamai coliu-akamai deleted the feat-m3-7227 branch November 6, 2023 19:44
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 VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants