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-7157] - Only show IPv4s specific to a linode-subnet relationship on VPC details page #9710

Merged
merged 5 commits into from
Sep 26, 2023

Conversation

coliu-akamai
Copy link
Contributor

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

Description 📝

Ensure that a linode row inside the subnets table only shows the IP specific to that subnet

Major Changes 🔄

Add subnetId to SubnetLinodeRow props, and find the interface associated with that specific subnet

Preview 📷

Before After
image image

MSW example:
image

How to test 🧪

How to verify changes?

  • Using the MSW:
    • Most linodes will have a blank VPC IPv4 column because the mock subnet_id of the mock linode's configs' interfaces don't match the subnet that the linode corresponds to. However, you can go to
      • packages/manager/src/factories/linodeConfigInterfaceFactory.ts line 28 and try to put in specific subnet_ids. If the mock subnet_id matches, the IPs will show up (see MSW screenshot example)
  • If you are not using the MSW, make sure you're on the dev environment and have the correct VPC flags associated with your account (may need to add them via admin). Navigate to a VPC's detail page and assign a linode to multiple subnets (note -- you'll have to either make the requests directly to the API, or first checkout my branch feat-m3-6752 to assign linodes from the UI, then switch back to this branch, until that gets merged in 😅)
    • verify that even if a linode is assigned to multiple subnets, only one IPv4 is shown in each row
    • verify that this is the correct IP for the specific linode-subnet relationship
      • (may need to console.log the configs in packages/manager/src/features/VPCs/VPCDetail/SubnetLinodeRow.tsx, and check the interfaces in the configs. The interface with the corresponding subnetId should have the same IPv4 as the one appearing in the row)

How to run Unit or E2E tests?
yarn test SubnetLinodeRow (existing test should still pass)
yarn test 'packages/manager/src/features/VPCs/utils.test.ts'

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Sep 21, 2023
@coliu-akamai coliu-akamai self-assigned this Sep 22, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review September 22, 2023 19:14
@coliu-akamai coliu-akamai requested a review from a team as a code owner September 22, 2023 19:14
@coliu-akamai coliu-akamai requested review from bnussman-akamai and abailly-akamai and removed request for a team September 22, 2023 19:14
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Logic looks good. From the mocking I tried, I saw the correct VPC IP!

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Sep 22, 2023
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

correct IP showing as expected in the UI ✅

Approving pending additional coverage. thx!

}
}

return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to a .utils.ts and get it tested with a factory?

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed the correct IP and that the existing unit test still passes 🔨

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 25, 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.

The VPC IPv4 displayed matches the IPv4 address expected ✅
Unit tests pass ✅

@coliu-akamai
Copy link
Contributor Author

thanks everyone! merging this PR as the integration test failure is unrelated to VPC changes

@coliu-akamai coliu-akamai merged commit d920fdf into linode:develop Sep 26, 2023
11 of 13 checks passed
@coliu-akamai coliu-akamai deleted the feat-m3-subnetlinode-row branch October 6, 2023 20:13
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.

5 participants