-
Notifications
You must be signed in to change notification settings - Fork 22
Add Edit Network Interface Modal #985
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
Changes from all commits
88b5f8f
8d0ff7d
6cd9302
ba466d8
9d262c9
30648c2
0d74f4a
63e24c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import invariant from 'tiny-invariant' | ||
|
|
||
| import type { NetworkInterface, NetworkInterfaceUpdate } from '@oxide/api' | ||
| import { useApiMutation, useApiQueryClient } from '@oxide/api' | ||
|
|
||
| import { DescriptionField, Form, NameField, SideModalForm } from 'app/components/form' | ||
| import type { EditSideModalFormProps } from 'app/forms' | ||
| import { useParams } from 'app/hooks' | ||
|
|
||
| export default function EditNetworkInterfaceSideModalForm({ | ||
| id = 'edit-network-interface-form', | ||
| title = 'Edit network interface', | ||
| onSubmit, | ||
| onSuccess, | ||
| onError, | ||
| onDismiss, | ||
| initialValues, | ||
| ...props | ||
| }: EditSideModalFormProps<NetworkInterfaceUpdate, NetworkInterface>) { | ||
| const queryClient = useApiQueryClient() | ||
| const pathParams = useParams('orgName', 'projectName') | ||
|
|
||
| const editNetworkInterface = useApiMutation('instanceNetworkInterfacesPutInterface', { | ||
| onSuccess(data) { | ||
| const { instanceName, ...others } = pathParams | ||
| invariant(instanceName, 'instanceName is required when posting a network interface') | ||
| queryClient.invalidateQueries('instanceNetworkInterfacesGet', { | ||
| instanceName, | ||
| ...others, | ||
| }) | ||
| onSuccess?.(data) | ||
| onDismiss() | ||
| }, | ||
| onError, | ||
| }) | ||
|
|
||
| return ( | ||
| <SideModalForm | ||
| id={id} | ||
| title={title} | ||
| initialValues={initialValues} | ||
| onDismiss={onDismiss} | ||
| onSubmit={ | ||
| onSubmit || | ||
| ((body) => { | ||
| const { instanceName, ...others } = pathParams | ||
| const interfaceName = initialValues.name | ||
| invariant( | ||
| interfaceName, | ||
| 'interfaceName is required when updating a network interface' | ||
| ) | ||
| invariant( | ||
| instanceName, | ||
| 'instanceName is required when posting a network interface' | ||
| ) | ||
|
|
||
| editNetworkInterface.mutate({ | ||
| instanceName, | ||
| interfaceName, | ||
| ...others, | ||
| body, | ||
| }) | ||
| }) | ||
| } | ||
| submitDisabled={editNetworkInterface.isLoading} | ||
| error={editNetworkInterface.error?.error as Error | undefined} | ||
| {...props} | ||
| > | ||
| <NameField id="nic-name" /> | ||
| <DescriptionField id="nic-description" /> | ||
| <Form.Submit>Save changes</Form.Submit> | ||
| </SideModalForm> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { test } from '@playwright/test' | ||
|
|
||
| import { expectNotVisible, expectVisible } from 'app/util/e2e' | ||
| import { expectNotVisible, expectRowVisible, expectVisible } from 'app/util/e2e' | ||
|
|
||
| test("Click through everything and make it's all there", async ({ page }) => { | ||
| await page.goto('/') | ||
|
|
@@ -107,7 +107,13 @@ test("Click through everything and make it's all there", async ({ page }) => { | |
|
|
||
| // Instance networking tab | ||
| await page.click('role=tab[name="Networking"]') | ||
| await expectVisible(page, ['role=cell[name="my-nic"]']) | ||
| await expectRowVisible(page, 'my-nic', [ | ||
| '', | ||
| 'my-nic', | ||
| 'a network interface', | ||
| '172.30.0.10', | ||
| 'primary', | ||
| ]) | ||
| await page.click('role=button[name="Add network interface"]') | ||
|
|
||
| // Add network interface | ||
|
|
@@ -129,15 +135,41 @@ test("Click through everything and make it's all there", async ({ page }) => { | |
| await page.click('role=button[name="Add network interface"]') | ||
| await expectVisible(page, ['role=cell[name="nic-2"]']) | ||
|
|
||
| // Delete just-added network interface | ||
| // Make this interface primary | ||
| await page | ||
| .locator('role=row', { hasText: 'nic-2' }) | ||
| .locator('role=button[name="Row actions"]') | ||
| .click() | ||
| await page.click('role=menuitem[name="Make primary"]') | ||
| await expectRowVisible(page, 'my-nic', [ | ||
| '', | ||
| 'my-nic', | ||
| 'a network interface', | ||
| '172.30.0.10', | ||
| '', | ||
| ]) | ||
| await expectRowVisible(page, 'nic-2', ['', 'nic-2', null, null, 'primary']) | ||
|
|
||
| // Make an edit to the network interface | ||
| await page | ||
| .locator('role=row', { hasText: 'nic-2' }) | ||
| .locator('role=button[name="Row actions"]') | ||
| .click() | ||
| await page.click('role=menuitem[name="Edit"]') | ||
| await page.fill('role=textbox[name="Name"]', 'nic-3') | ||
| await page.click('role=button[name="Save changes"]') | ||
| await expectNotVisible(page, ['role=cell[name="nic-2"]']) | ||
| await expectVisible(page, ['role=cell[name="nic-3"]']) | ||
|
|
||
| // Delete just-added network interface | ||
| await page | ||
| .locator('role=row', { hasText: 'nic-3' }) | ||
| .locator('role=button[name="Row actions"]') | ||
| .click() | ||
| await page.click('role=menuitem[name="Delete"]') | ||
| // Close toast, it holds up the test for some reason | ||
| await page.click('role=button[name="Dismiss notification"]') | ||
| await expectNotVisible(page, ['role=cell[name="nic-2"]']) | ||
| await expectNotVisible(page, ['role=cell[name="nic-3"]']) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. love all this |
||
|
|
||
| // Snapshots page | ||
| await page.click('role=link[name*="Snapshots"]') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,21 @@ | ||
| import { useState } from 'react' | ||
|
|
||
| import type { NetworkInterface } from '@oxide/api' | ||
| import type { NetworkInterface, NetworkInterfaceUpdate } from '@oxide/api' | ||
| import { useApiMutation, useApiQuery, useApiQueryClient } from '@oxide/api' | ||
| import type { MenuAction } from '@oxide/table' | ||
| import { useQueryTable } from '@oxide/table' | ||
| import { | ||
| Badge, | ||
| Button, | ||
| Delete16Icon, | ||
| EmptyMessage, | ||
| Info16Icon, | ||
| Networking24Icon, | ||
| Tooltip, | ||
| OpenLink12Icon, | ||
| Success12Icon, | ||
| } from '@oxide/ui' | ||
|
|
||
| import CreateNetworkInterfaceSideModalForm from 'app/forms/network-interface-create' | ||
| import EditNetworkInterfaceSideModalForm from 'app/forms/network-interface-edit' | ||
| import { useParams, useToast } from 'app/hooks' | ||
|
|
||
| export function NetworkingTab() { | ||
|
|
@@ -22,6 +24,7 @@ export function NetworkingTab() { | |
| const addToast = useToast() | ||
|
|
||
| const [createModalOpen, setCreateModalOpen] = useState(false) | ||
| const [editing, setEditing] = useState<NetworkInterfaceUpdate | null>(null) | ||
|
|
||
| const getQuery = ['instanceNetworkInterfacesGet', instanceParams] as const | ||
|
|
||
|
|
@@ -36,12 +39,42 @@ export function NetworkingTab() { | |
| }, | ||
| }) | ||
|
|
||
| const editNic = useApiMutation('instanceNetworkInterfacesPutInterface', { | ||
| onSuccess() { | ||
| queryClient.invalidateQueries(...getQuery) | ||
| }, | ||
| }) | ||
|
|
||
| const instanceStopped = | ||
| useApiQuery('projectInstancesGetInstance', instanceParams).data?.runState === 'stopped' | ||
|
|
||
| const makeActions = (nic: NetworkInterface): MenuAction[] => [ | ||
| { | ||
| label: 'Make primary', | ||
| onActivate() { | ||
| editNic.mutate({ | ||
| ...instanceParams, | ||
| interfaceName: nic.name, | ||
| body: { ...nic, makePrimary: true }, | ||
| }) | ||
| }, | ||
| disabled: nic.primary || !instanceStopped, | ||
| }, | ||
| { | ||
| label: 'Edit', | ||
| onActivate() { | ||
| // TODO: Revisit after https://github.com/oxidecomputer/omicron/pull/1288 is merged | ||
| setEditing({ ...nic, makePrimary: nic.primary }) | ||
| }, | ||
| disabled: !instanceStopped, | ||
| }, | ||
| { | ||
| label: 'Delete', | ||
| className: 'destructive', | ||
| onActivate: () => { | ||
| deleteNic.mutate({ ...instanceParams, interfaceName: nic.name }) | ||
| }, | ||
| disabled: !instanceStopped, | ||
| }, | ||
| ] | ||
|
|
||
|
|
@@ -55,48 +88,60 @@ export function NetworkingTab() { | |
| /> | ||
| ) | ||
|
|
||
| const instanceStopped = | ||
| useApiQuery('projectInstancesGetInstance', instanceParams).data?.runState === 'stopped' | ||
|
|
||
| const { Table, Column } = useQueryTable(...getQuery) | ||
| return ( | ||
| <> | ||
| <div className="mb-3 flex items-center justify-end space-x-4"> | ||
| { | ||
| // TODO: update icon color | ||
| // TODO: the tooltip pops up on the right edge of the icon instead of | ||
| // the middle, wtf. not worth fixing because we're going to redo | ||
| // Tooltip anyway | ||
| // TODO: would be cool to also show the tooltip on button hover when it's disabled | ||
| !instanceStopped && ( | ||
| <Tooltip | ||
| id="add-nic-tooltip" | ||
| content="A network interface cannot be added unless the instance is stopped." | ||
| > | ||
| <Info16Icon className="cursor-default text-secondary" /> | ||
| </Tooltip> | ||
| ) | ||
| } | ||
| <Button | ||
| size="xs" | ||
| variant="default" | ||
| onClick={() => setCreateModalOpen(true)} | ||
| disabled={!instanceStopped} | ||
| > | ||
| Add network interface | ||
| </Button> | ||
| <CreateNetworkInterfaceSideModalForm | ||
| isOpen={createModalOpen} | ||
| onDismiss={() => setCreateModalOpen(false)} | ||
| onSuccess={() => setCreateModalOpen(false)} | ||
| /> | ||
| </div> | ||
| <h2 id="network-interfaces" className="mb-4 text-mono-sm text-secondary"> | ||
| Network Interfaces | ||
| </h2> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll leave that for a polish follow up |
||
| <Table makeActions={makeActions} emptyState={emptyState}> | ||
| <Column accessor="name" /> | ||
| <Column accessor="description" /> | ||
| {/* TODO: mark v4 or v6 explicitly? */} | ||
| <Column accessor="ip" /> | ||
| <Column | ||
| accessor="primary" | ||
| cell={({ value }) => | ||
| value && ( | ||
| <> | ||
| <Success12Icon className="text-accent mr-1" /> | ||
| <Badge variant="secondary">primary</Badge> | ||
| </> | ||
| ) | ||
| } | ||
| /> | ||
| </Table> | ||
| <div className="mt-4 flex flex-col gap-3"> | ||
| <div className="flex gap-3"> | ||
| <Button | ||
| size="xs" | ||
| variant="default" | ||
| onClick={() => setCreateModalOpen(true)} | ||
| disabled={!instanceStopped} | ||
| > | ||
| Add network interface | ||
| </Button> | ||
| </div> | ||
| {!instanceStopped && ( | ||
| <span className="max-w-xs text-sans-sm text-secondary"> | ||
| A network interface cannot be created or edited without{' '} | ||
| <a href="#/" className="text-accent-secondary"> | ||
| stopping the instance | ||
| <OpenLink12Icon className="ml-1 pt-[1px]" /> | ||
| </a> | ||
| </span> | ||
| )} | ||
| </div> | ||
|
|
||
| <CreateNetworkInterfaceSideModalForm | ||
| isOpen={createModalOpen} | ||
| onDismiss={() => setCreateModalOpen(false)} | ||
| /> | ||
| <EditNetworkInterfaceSideModalForm | ||
| isOpen={!!editing} | ||
| initialValues={editing || {}} | ||
| onDismiss={() => setEditing(null)} | ||
| /> | ||
| </> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,11 +32,15 @@ export async function expectNotVisible(page: Page, selectors: string[]) { | |
| export async function expectRowVisible( | ||
| page: Page, | ||
| rowSelectorText: string, | ||
| cellTexts: string[] | ||
| cellTexts: Array<string | null> | ||
| ) { | ||
| const row = page.locator(`tr:has-text("${rowSelectorText}")`) | ||
| await expect(row).toBeVisible() | ||
| for (let i = 0; i < cellTexts.length; i++) { | ||
| await expect(row.locator(`role=cell >> nth=${i}`)).toHaveText(cellTexts[i]) | ||
| const text = cellTexts[i] | ||
| if (text === null) { | ||
| continue | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good change |
||
| await expect(row.locator(`role=cell >> nth=${i}`)).toHaveText(text) | ||
| } | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theuseParams()on line 21.I also don't see why we need an invariant on
interfaceNamesince it's a required field in the form — the invariant is enforced by form validation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's used in the
instance-create-form(via the custom NIC option) theninstanceNamewouldn't be in the path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the invariant on
interfaceName... this is mostly b/c the PUT method doesn't require any of these inputs. We useNetworkInterfaceUpdateto driveinitialValueswhich hasnametyped asstring | null | undefined. The modal is invoked like below, notice thatinitialValuesis potentially an empty object.