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-6729] - Add VPC column to Linodes landing table #9485

Merged
merged 23 commits into from
Aug 15, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Aug 2, 2023

Description 📝

Added vpc column to linode landing table so that users can see what vpc a linode is associated with

Major Changes 🔄

  • add vpc column to linode landing table, add functionality to include vpc information (vpc's label & id) with a linode's data
  • changed IP Address column to public iP address

Preview 📷

image

How to test 🧪

  1. Turn on mocks and navigate to linodes landing page to see the vpc column (you can see the vpc column without the mock as long as you have the vpc flag enabled, but you won't be able to see any mock data). Clicking on a vpc label should bring you to the vpc detail page (currently blank / a todo) :D

Comment on lines 80 to 82
// a linode should have only one vpc associated with it (?) so we can
// short circuit once we find the associated vpc
return vpc;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was seeing in the mock examples that linodes would often have multiple vpcs associated with them, but just wanted to confirm that in the actual live data, a linode will have at most one vpc associated with it (although this function may eventually be unnecessary if the api endpoints get updated)

packages/manager/src/utilities/linodes.ts Outdated Show resolved Hide resolved
packages/manager/src/features/Linodes/index.tsx Outdated Show resolved Hide resolved
Comment on lines 50 to 56
const { data: accountMaintenanceData } = useAllAccountMaintenanceQuery(
{},
{ status: { '+or': ['pending, started'] } }
);

const maintenance = accountMaintenanceData?.find(
(m) => m.entity.id === id && m.entity.type === 'linode'
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this bc we seem to already have this information associated with the linodes due to this functionality being done in src/features/Linodes/index.tsx and the information needed for the maintenance being passed along from the parent components, but lmk if I missed anything!

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Aug 3, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review August 3, 2023 18:30
@bnussman-akamai
Copy link
Member

@jaalah-akamai Should we build this right now given that the API might add /v4/linode/instances/:id/vpcs for us to use for this purpose?

@jaalah-akamai
Copy link
Contributor

I should probably clarify - API will not be adding a new endpoint at this time, but they will correct the existing endpoint /linode/instances/{linode id}/configs/{config id}/interfaces to provide the VPC id which is needed to more easily determine the VPC label for that column

@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Aug 4, 2023

@jaalah-akamai Should we build this right now given that the API might add /v4/linode/instances/:id/vpcs for us to use for this purpose?

I will also add this fix is pending API approval, so I'm for moving forward with the approach we know will work (even though it's a roundabout way of getting the VPC label).

Update: This will be out in Alpha by Tuesday, so let's wait to finish this work

@bnussman-akamai
Copy link
Member

bnussman-akamai commented Aug 4, 2023

So for every Linode row we'll have to do a GET /v4/linode/instances/{linode id}/configs and a GET /v4/linode/instances/{linode id}/configs/{config id}/interfaces for every config? That isn't good. At that point, Connie original solution would be much better.

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Aug 4, 2023

I think the API team's plan is to add the VPC id to the interface config return object, so just using GET /v4/linode/instances/{linode id}/configs to a get a list of all the configs should do the trick (and then I'd need to loop through every config to be able to loop through all their interfaces, and then just get the first interface with a purpose of vpc. From there I'd have to do a GET /vpcs/<vpc_id> so that I can also get the vpc's label). iirc, the config return object already has all the interface objects inside it

tbh, I might still need to put this functionality into the Linodes/index.tsx file in order to be able to have the table sort by the vpc label -- earlier, when I'd had a solution of getting the vpc id and label inside the Linodes/LinodesLanding/LinodeRow/LinodeRow.tsx file only, the sorting didn't work. I can look into this more once the new api spec comes out on Tuesday though! 👀

@bnussman-akamai
Copy link
Member

Okay, understood @coliu-akamai.

I do want to bring some attention to making vpc label sortable on the client-side. I don't think we should let people order by vpc label. Practices like this have been blocking us from API paginating and sorting Linodes. I think the only way we should allow this is if the /linode/instances endpoint returns a vpc_label , which would mean the API can sort by it on the server-side. @jaalah-akamai

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.

Left a couple of comments -- I would agree with not allowing for sorting by the VPC label in the Linodes landing table unless the API supports it.

@coliu-akamai
Copy link
Contributor Author

updated to reflect the new api changes -- have also made the vpc column not sortable

@coliu-akamai coliu-akamai added @linode/api-v4 Changes are made to the @linode/api-v4 package @linode/validation Changes are made to the @linode/validation package labels Aug 11, 2023
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.

Looking good, just have a few small items before I approve ✅

<TableCell noWrap>
{vpcLabel && (vpcId === 0 || vpcId) && (
<Link tabIndex={0} to={`/vpc/${vpcId}`}>
{vpcLabel}
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some kind of state for when there is no VPC attached?

Screenshot 2023-08-14 at 1 45 44 PM
Screenshot 2023-08-14 at 1 52 21 PM

Copy link
Member

Choose a reason for hiding this comment

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

Along the same lines: because we have to make a lot of API calls now, should we add a <Skeleton /> as a placeholder while we load the data? We do this for NodeBalancer Ports and it looks nice.

Screenshot 2023-08-14 at 1 56 52 PM

coliu-akamai and others added 2 commits August 14, 2023 14:11
…/LinodeRow.tsx

Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>
Comment on lines +186 to +193
{vpcLoading || configsLoading ? (
<Skeleton />
) : vpcLabel ? (
<Link tabIndex={0} to={`/vpc/${vpcId}`}>
{vpcLabel}
</Link>
) : (
'None'
Copy link
Contributor Author

@coliu-akamai coliu-akamai Aug 14, 2023

Choose a reason for hiding this comment

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

@bnussman-akamai added the skeleton / 'None'! I think the skeleton is definitely a nice addition for loading. The 'none' looked good too imo -- would we need to get UX approval for this?

Copy link
Member

Choose a reason for hiding this comment

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

I think it looks great! We probably should get UX approval. Can you reach out to Andrew?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Andrew approved! 🎉

@jdamore-linode
Copy link
Contributor

jdamore-linode commented Aug 14, 2023

Hey @coliu-akamai, just a heads up that there's an E2E test failure! It's nothing big, just a test expecting to see "IP Address" rather than "Public IP Address" in the Linodes landing page. The failure is in manager/cypress/core/e2e/linodes/smoke-linode-landing-table.spec.ts (and the specific test is checks the table and action menu buttons/labels).

You can fix that test and verify your changes with this command while Cloud's running locally:

yarn cy:run -s "cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts"

Happy to help out if you have any trouble!

@coliu-akamai
Copy link
Contributor Author

@jdamore-linode thanks for pointing that out! Those tests are now passing on local so hopefully they pass here too 🎉

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.

Nice job! 🚀

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

@jdamore-linode jdamore-linode 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, @coliu-akamai !

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

Good work on this! There's one cleanup item but I'm approving pending that being addressed.

packages/validation/src/linodes.schema.ts Outdated Show resolved Hide resolved
@coliu-akamai coliu-akamai merged commit 2b55810 into linode:develop Aug 15, 2023
11 checks passed
@coliu-akamai coliu-akamai deleted the feat-m3-6729 branch August 25, 2023 03:49
corya-akamai pushed a commit to corya-akamai/manager that referenced this pull request Sep 6, 2023
* add vpc column to linode landing pg table

* roundabout way to get vpcs associated with a linode

* add comments and todos

* sortable table row for vpcs now

* remove unneeded comment

* ...fix comment

* changeset and update test

* linode row trying to test for vpc column wip

* test vpc column shows up with feature flag

* updated linode row vpc column

* Added changeset: Update linode config interface return object as per new API spec

* Update packages/api-v4/.changeset/pr-9485-upcoming-features-1691764196950.md

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

* Update packages/manager/src/features/Linodes/LinodesLanding/LinodeRow/LinodeRow.tsx

Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>

* address feedback

* Added changeset: Update validation for `linodeInterfaceSchema`: include validation for `vpc_id` and update `subnet` to `subnet_id`

* Update packages/manager/src/features/Linodes/LinodesLanding/LinodeRow/LinodeRow.tsx

Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>

* address feedback @bnussman-akamai

* update cypress test

* remove vpc_id from linode interface schema

---------

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.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.

5 participants