Add Edit Network Interface Modal

[just-be-dev]

Fixes #933

Changes

• Updated the NIC table to show which is primary

• Added edit form

It's notable that Primary is disabled if the NIC you're editing is already primary. @benjaminleonard this could probably use a better design. Should I put a field title over it?

TODO

• Add E2E tests
• Add tooltip to primary checkbox and make the UI not suck

[vercel]

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

Name	Status	Preview	Updated
console-ui-storybook	✅ Ready (Inspect)	Visit Preview	Jun 28, 2022 at 8:43PM (UTC)

[benjaminleonard]

Yeah we should include a field title, a description, and a tooltip
I'm also wondering if we should use a radio button for this. I think I'll mock something up, because it's probably useful to see the consequences of your selection...i.e. if this becomes the primary NIC the other/s becomes....I don't know.

Primary also implies an order. If you had three NICs, are two of them secondary?

Also worth saying we don't currently have a disabled state for a selected checkbox. I'm thinking of using opacity for disabled states instead of the fixed colours.

Edit: The idea of disabling it because its already primary feels unusual to me, and editing other NICs within another NIC's pane also. Since to make one primary, you would need to demote another.

Perhaps better suited for a modal action outside of the pane, as in a button that is "Set Primary NIC", with radio buttons or a dropdown.

[benjaminleonard]

and it goes a lil' something like this (ignore that it's selecting the current selected item)

with edit pane - not sure what we call the 'Primary' field. How is this reflected in the API? A boolean?

[bnaecker]

A radio button certainly communicates the exclusivity. And yes, there’s exactly one primary interface, and all others are secondary. I like that UI, keeping the setting of the primary distinct from the other updates.

…

On 28 Jun 2022, at 03:19, Benjamin Leonard ***@***.***> wrote:  and it goes a lil' something like this (ignore that it's selecting the current selected item) — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

[just-be-dev]

@benjaminleonard, primary is a Boolean, yes.

[benjaminleonard]

@benjaminleonard, primary is a Boolean, yes.

Cool – I ask because the column name is Primary, I like to have text not just the checkmark icon so we're not relying entirely on the icon for meaning. Not that it's particularly ambiguous.

So currently it says Primary: Primary. Idk if there's a better way of doing that, it's probably fine for now.

[just-be-dev]

Oh, also, we can't change the ip address either. It's only the name and description that are editable.

[just-be-dev]

I've added the action to make a NIC primary in the row actions menu. I'll create an issue to do a follow up. The are questions around the modal approach like what columns besides name would be needed in the modal to make the decision to switch which is primary. We also need to figure out how to manage what's the primary interface on instance create when making a custom NIC.

[bnaecker]

We also need to figure out how to manage what's the primary interface on instance create when making a custom NIC.

The primary NIC is always the first one, if multiple are specified at instance-creation time. Really, the first interface to be attached to an instance is the primary, no matter when they're specified (instance create, or later with new requests to add a NIC to an instance).

[just-be-dev]

I updated the UI of the table itself to be closer to the storage tab (complete with text that says editing can only happen when the instance is stopped)

[david-crespo]

love all this

[david-crespo]

This matches what's on storage, and I agree we need something to say what's in the table, but I think it's too small (which probably means the disks ones are too small too?) @benjaminleonard

[just-be-dev]

I'll leave that for a polish follow up

[david-crespo]

good change

[david-crespo]

Do you anticipate using this form in a context where we don't have an instanceName? Otherwise this is basically the same as including 'instanceName' in the useParams() on line 21.

I also don't see why we need an invariant on interfaceName since it's a required field in the form — the invariant is enforced by form validation.

[just-be-dev]

If it's used in the instance-create-form (via the custom NIC option) then instanceName wouldn't be in the path.

[just-be-dev]

As for the invariant on interfaceName... this is mostly b/c the PUT method doesn't require any of these inputs. We use NetworkInterfaceUpdate to drive initialValues which has name typed as string | null | undefined. The modal is invoked like below, notice that initialValues is potentially an empty object.

      <EditNetworkInterfaceSideModalForm
        isOpen={!!editing}
        initialValues={editing || {}}
        onDismiss={() => setEditing(null)}
      />

[david-crespo]

Edit: this is unrelated to this PR, made it its own issue #993

---

These lines in the mock NIC create endpoint (which were already there) mean that any NIC create without a VPC name and a subnet name will 404.

console/libs/api-mocks/msw/handlers.ts

Lines 491 to 499 in 1f746d5

	const [vpc, vpcErr] = lookupVpc({ ...req.params, vpcName: vpc_name })
	if (vpcErr) return res(vpcErr)
	const [subnet, subnetErr] = lookupVpcSubnet({
	...req.params,
	vpcName: vpc_name,
	subnetName: subnet_name,
	})
	if (subnetErr) return res(subnetErr)

Turns out these are not actually optional on the POST, which is a relief because I did not understand how that would work. So those fields should be marked required in the create form.

https://github.com/oxidecomputer/omicron/blob/a0e5d59a3190b40138c5d6928f6cb89aebb00e9b/nexus/src/external_api/params.rs#L275-L287

[just-be-dev]

I'm going to merge to move forward, but again I have #991 for updates. If anything else comes up in post review, let me know and I'll add it to that issue as a follow up.