Add VPC and instance external IP to instance page #1882

benjaminleonard · 2024-01-15T12:25:30Z

Some assumptions here: we're just interested in showing the primary VPC here, but we might want to show all of the external IPs.

Added VPC here also because whilst the current navigation to get to the associated VPC feels systematically correct, we have had people struggle to find their firewall rules and this is part of a larger effort to make that easier to find.

Also making it clearer they are clickable with the underline (following up with some similar changes elsewhere to make other links clearer).

Also fixes #1775 through #1885

vercel · 2024-01-15T12:25:35Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Jan 17, 2024 10:17pm

benjaminleonard · 2024-01-15T12:28:56Z

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

@@ -52,10 +52,11 @@ const VpcNameFromId = ({ value }: { value: string }) => {
  // possible because you can't delete a VPC that has child resources, but let's
  // be safe
  if (isError) return <Badge color="neutral">Deleted</Badge>
-  if (!vpc) return <Spinner /> // loading
+  if (!vpc)
+    return <div className="h-4 w-12 rounded bg-tertiary motion-safe:animate-pulse" /> // loading


Using a skeleton state here rather than the full spinner which can be a bit strong.

I didn't know we had that. That's great. You should update SubnetNameFromId too, which is also using the spinner. Also maybe make it wider (maybe not the pulsing thing itself, but its container) so the pop-in is less severe when the name shows up? Though that depends on making a guess about the average length of name. Usually it will be default, right?

benjaminleonard · 2024-01-15T12:31:04Z

Perhaps also beyond this PR, but would be good to have a mechanism to truncate arrays of strings. See:

* Update copy to clipboard component Uses new icon and snazzy animation * Add copy to clipboard to external IP * `CopyOnTruncate` component * Add description to `CopyOnTruncate` * Revert "Add description to `CopyOnTruncate`" This reverts commit 4bf0bcc. * Revert "`CopyOnTruncate` component" This reverts commit e4a0107.

david-crespo · 2024-01-16T18:04:58Z

Perhaps also beyond this PR, but would be good to have a mechanism to truncate arrays of strings. See:

Are you picturing a truncate with "show more" button? In this spot I think it would be fine if it wrapped and stretched the box vertically if the user asked for it. Though as the thing is written I think it would mess up the grid.

app/pages/project/instances/instance/InstancePage.tsx

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

github-actions

license-eye has totally checked 459 files.

Valid	Invalid	Ignored	Fixed
407	1	51	0

Click to see the invalid file list

libs/api-mocks/external-ip.ts

libs/api-mocks/external-ip.ts

david-crespo · 2024-01-16T18:56:37Z

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

+
+  return (
+    <div className="flex items-center gap-1 text-secondary">
+      {primary ? ips : <>&mdash;</>}


a) this logic doesn't work for the case where you're looking at the whole list and don't have any IPs. primary && ips.length > 0 would probably cover it, but deserves a comment to explain that the two conditions are for two different call sites. I might tweak the props API since primary is not relevant to the instance page and the need to include primary: true there is confusing because it's meaningless.

b) should probably make the VPC one also use mdash

fixed it I think

benjaminleonard · 2024-01-17T09:39:08Z

libs/ui/lib/properties-table/PropertiesTable.tsx

@@ -45,7 +45,7 @@ PropertiesTable.Row = ({ label, children }: PropertiesTableRowProps) => (
    <span className="flex items-center">
      <Badge>{label}</Badge>
    </span>
-    <div className="flex items-center overflow-hidden whitespace-nowrap pr-4 text-sans-md text-secondary">
+    <div className="flex h-11 items-center overflow-hidden whitespace-nowrap pr-4 text-sans-md text-secondary">


We were getting a flash of movement when the skeleton switched to the text since they're not the exact same height. This fixes that and any future issues where the content of the row is not the same height as text-sans-md.

benjaminleonard · 2024-01-17T09:43:49Z

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

-const VpcNameFromId = ({ value }: { value: string }) => {
+export const Skeleton = classed.div`h-4 w-12 rounded bg-tertiary motion-safe:animate-pulse`
+
+export const EmptyCellContent = () => (


Might be a better home for this?

benjaminleonard · 2024-01-17T09:44:30Z

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

+  return (
+    <a
+      className="underline text-sans-semi-md text-secondary hover:text-default"
+      href={ip}


This needs a protocol – we can assume this is http right?

Not sure. Might be better to do https and let them fix it after nav if it’s wrong.

david-crespo · 2024-01-17T18:14:52Z

app/pages/project/instances/instance/tabs/NetworkingTab.tsx

-          cell={ExternalIpsFromInstanceName}
+          cell={({ value: primary }) =>
+            primary ? <ExternalIps {...instanceSelector} /> : <EmptyCell />
+          }


This primary logic was inside the component before, but it's only relevant here, not in the table at the top. But you know... what if we just got rid of this column? The relation to the network interface is kind of tenuous to begin with. VPC name is different because each network interface can point to a different VPC. But external IPs really are instance level. So let's take them out of this table?

I think it's an improvement! One other thing we could do is put a tooltip on IP to explain that it's an internal IP?

david-crespo · 2024-01-17T19:37:40Z

app/pages/project/instances/instance/InstancePage.tsx

@@ -81,6 +116,8 @@ export function InstancePage() {

  const memory = filesize(instance.memory, { output: 'object', base: 2 })

+  const primaryVpcId = nics.items.length > 0 ? nics.items[0].vpcId : undefined
+


It just occurred to me — I don't think we can rely on the first NIC being the primary one. We have to find the primary directly.

oxidecomputer/console@367142c...644a45b * [644a45b8](oxidecomputer/console@644a45b8) oxidecomputer/console#1882 * [69927db1](oxidecomputer/console@69927db1) oxidecomputer/console#1879 * [a3165dff](oxidecomputer/console@a3165dff) oxidecomputer/console#1883 * [4f706c2b](oxidecomputer/console@4f706c2b) oxidecomputer/console#1877 * [7999343d](oxidecomputer/console@7999343d) oxidecomputer/console#1880

Add VPC and instance external IP to instance page

ee0bf86

benjaminleonard commented Jan 15, 2024

View reviewed changes

Use slash as divider

31c0d28

vercel bot deployed to Preview January 15, 2024 12:31 View deployment

vercel bot deployed to Preview January 15, 2024 16:57 View deployment

benjaminleonard mentioned this pull request Jan 15, 2024

Update dependency @oxide/design-system to v1.2.10 #1884

Closed

1 task

david-crespo reviewed Jan 16, 2024

View reviewed changes

app/pages/project/instances/instance/InstancePage.tsx Outdated Show resolved Hide resolved

david-crespo reviewed Jan 16, 2024

View reviewed changes

app/pages/project/instances/instance/tabs/NetworkingTab.tsx Outdated Show resolved Hide resolved

basic externalIps table. every instance still has the same IP though

0e16ec2

vercel bot deployed to Preview January 16, 2024 18:43 View deployment

github-actions bot reviewed Jan 16, 2024

View reviewed changes

libs/api-mocks/external-ip.ts Show resolved Hide resolved

make external IPs legit

b20ce6c

vercel bot deployed to Preview January 16, 2024 18:51 View deployment

add file license

9779f97

vercel bot deployed to Preview January 16, 2024 18:53 View deployment

david-crespo reviewed Jan 16, 2024

View reviewed changes

fix fallback logic around no IPs

acceef9

vercel bot deployed to Preview January 16, 2024 19:15 View deployment

skeleton cell component

4fa26fd

vercel bot deployed to Preview January 16, 2024 20:08 View deployment

Give properties row a fixed height

9a74c92

vercel bot deployed to Preview January 17, 2024 09:38 View deployment

benjaminleonard commented Jan 17, 2024

View reviewed changes

EmptyCellContent component instead of — directly

c8e2cc8

benjaminleonard commented Jan 17, 2024

View reviewed changes

vercel bot deployed to Preview January 17, 2024 09:43 View deployment

benjaminleonard commented Jan 17, 2024

View reviewed changes

david-crespo added 2 commits January 17, 2024 12:10

extract ExternalIps component that doesn't know about primary

bd14bdc

put https on IP link

2088e91

vercel bot deployed to Preview January 17, 2024 18:12 View deployment

david-crespo reviewed Jan 17, 2024

View reviewed changes

david-crespo added 3 commits January 17, 2024 12:57

remove external IP column from nics table, add asserts to e2e test

8d582c9

put prefetches in Promise.all

79c9839

sneaky prefetch on VPC name

3d5d4b8

vercel bot deployed to Preview January 17, 2024 19:35 View deployment

david-crespo reviewed Jan 17, 2024

View reviewed changes

share primary VPC ID logic

c31c850

vercel bot deployed to Preview January 17, 2024 22:17 View deployment

david-crespo merged commit 644a45b into main Jan 17, 2024

david-crespo deleted the add-instance-ip-properties branch January 17, 2024 22:30

david-crespo mentioned this pull request Jan 17, 2024

Bump web console oxidecomputer/omicron#4830

Merged

david-crespo mentioned this pull request Jan 29, 2024

Make VpcNameFromId more linkCell-like #1918

Merged

		@@ -81,6 +116,8 @@ export function InstancePage() {

		const memory = filesize(instance.memory, { output: 'object', base: 2 })

		const primaryVpcId = nics.items.length > 0 ? nics.items[0].vpcId : undefined

Add VPC and instance external IP to instance page #1882

Add VPC and instance external IP to instance page #1882

Uh oh!

Conversation

benjaminleonard commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminleonard commented Jan 15, 2024

Uh oh!

david-crespo commented Jan 16, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-crespo Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjaminleonard commented Jan 15, 2024 •

edited

Loading

vercel bot commented Jan 15, 2024 •

edited

Loading

david-crespo Jan 16, 2024 •

edited

Loading

david-crespo Jan 16, 2024 •

edited

Loading