From eb65a5c598c01d312a0753572850d6349c574c1f Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 14:18:30 -0800 Subject: [PATCH 01/10] Disallow duplicate disk names in instance create flow --- app/components/form/fields/DisksTableField.tsx | 3 +++ app/forms/disk-create.tsx | 12 +++++++++++- app/forms/instance-create.tsx | 10 +++++++++- test/e2e/instance-create.e2e.ts | 10 +++++++++- 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/app/components/form/fields/DisksTableField.tsx b/app/components/form/fields/DisksTableField.tsx index 2c3c8f0559..2d9aa12708 100644 --- a/app/components/form/fields/DisksTableField.tsx +++ b/app/components/form/fields/DisksTableField.tsx @@ -30,9 +30,11 @@ export type DiskTableItem = export function DisksTableField({ control, disabled, + unavailableDiskNames, }: { control: Control disabled: boolean + unavailableDiskNames: string[] }) { const [showDiskCreate, setShowDiskCreate] = useState(false) const [showDiskAttach, setShowDiskAttach] = useState(false) @@ -108,6 +110,7 @@ export function DisksTableField({ onChange([...items, { type: 'create', ...values }]) setShowDiskCreate(false) }} + unavailableDiskNames={unavailableDiskNames} onDismiss={() => setShowDiskCreate(false)} /> )} diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 593056f4df..703e27cef0 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -64,12 +64,14 @@ type CreateSideModalFormProps = { */ onDismiss: (navigate: NavigateFunction) => void onSuccess?: (disk: Disk) => void + unavailableDiskNames: string[] } export function CreateDiskSideModalForm({ onSubmit, onSuccess, onDismiss, + unavailableDiskNames, }: CreateSideModalFormProps) { const queryClient = useApiQueryClient() const navigate = useNavigate() @@ -132,7 +134,15 @@ export function CreateDiskSideModalForm({ loading={createDisk.isPending} submitError={createDisk.error} > - + { + if (unavailableDiskNames.includes(name)) { + return 'Name is already in use' + } + }} + /> ) + const existingDiskNames = allDisks.map((disk) => disk.name) + const otherDisks = useWatch({ control, name: 'otherDisks' }).map((disk) => disk.name) + const unavailableDiskNames = [...existingDiskNames, ...otherDisks] + return ( <> @@ -561,7 +565,11 @@ export function CreateInstanceForm() { Additional disks - + Authentication diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index e2e2125100..c26de640f7 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -510,11 +510,19 @@ test('create instance with additional disks', async ({ page }) => { await page.getByRole('button', { name: 'Create new disk' }).click() const createForm = page.getByRole('dialog', { name: 'Create disk' }) - await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1') + + // verify that an existing name can't be used + await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('disk-6') await createForm.getByRole('textbox', { name: 'Size (GiB)' }).fill('5') await createForm.getByRole('button', { name: 'Create disk' }).click() + await expect(createForm.getByText('Name is already in use')).toBeVisible() + + // rename that failed disk + await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1') + await createForm.getByRole('button', { name: 'Create disk' }).click() const disksTable = page.getByRole('table', { name: 'Disks' }) + await expect(disksTable.getByText('disk-6')).toBeHidden() await expectRowVisible(disksTable, { Name: 'new-disk-1', Type: 'create', Size: '5GiB' }) // Attach an existing disk From 6478047a2059571cb57d3b0f68eb5f964d54b7ab Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Fri, 8 Nov 2024 15:17:45 -0800 Subject: [PATCH 02/10] Query for ALL_ISH disk list in create disk form --- app/forms/disk-create.tsx | 16 ++++++++++++---- app/forms/instance-create.tsx | 5 ++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 703e27cef0..2e9f6ec5c6 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -36,6 +36,7 @@ import { FieldLabel } from '~/ui/lib/FieldLabel' import { Radio } from '~/ui/lib/Radio' import { RadioGroup } from '~/ui/lib/RadioGroup' import { Slash } from '~/ui/lib/Slash' +import { ALL_ISH } from '~/util/consts' import { toLocaleDateString } from '~/util/date' import { bytesToGiB, GiB } from '~/util/units' @@ -64,18 +65,26 @@ type CreateSideModalFormProps = { */ onDismiss: (navigate: NavigateFunction) => void onSuccess?: (disk: Disk) => void - unavailableDiskNames: string[] + unavailableDiskNames?: string[] } export function CreateDiskSideModalForm({ onSubmit, onSuccess, onDismiss, - unavailableDiskNames, + unavailableDiskNames = [], }: CreateSideModalFormProps) { const queryClient = useApiQueryClient() const navigate = useNavigate() + const { project } = useProjectSelector() + const { data: allDisks } = useApiQuery('diskList', { query: { project, limit: ALL_ISH } }) + // There might be duplicates here, but that's fine; inclides() will early return on the first match + const allUnavailableDiskNames = [ + ...(allDisks?.items.map((d) => d.name) || []), + ...unavailableDiskNames, + ] + const createDisk = useApiMutation('diskCreate', { onSuccess(data) { queryClient.invalidateQueries('diskList') @@ -86,7 +95,6 @@ export function CreateDiskSideModalForm({ }) const form = useForm({ defaultValues }) - const { project } = useProjectSelector() const projectImages = useApiQuery('imageList', { query: { project } }) const siloImages = useApiQuery('imageList', {}) @@ -138,7 +146,7 @@ export function CreateDiskSideModalForm({ name="name" control={form.control} validate={(name: string) => { - if (unavailableDiskNames.includes(name)) { + if (allUnavailableDiskNames.includes(name)) { return 'Name is already in use' } }} diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 728cd6c88d..509b6dc698 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -277,9 +277,8 @@ export function CreateInstanceForm() { ) - const existingDiskNames = allDisks.map((disk) => disk.name) - const otherDisks = useWatch({ control, name: 'otherDisks' }).map((disk) => disk.name) - const unavailableDiskNames = [...existingDiskNames, ...otherDisks] + const otherDisks = useWatch({ control, name: 'otherDisks' }) + const unavailableDiskNames = otherDisks.map((disk) => disk.name) return ( <> From 65e738781be9612c5f2893ab118019796e22d336 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 13:26:14 -0800 Subject: [PATCH 03/10] typo --- app/forms/disk-create.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 2e9f6ec5c6..157e0463ca 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -79,7 +79,7 @@ export function CreateDiskSideModalForm({ const { project } = useProjectSelector() const { data: allDisks } = useApiQuery('diskList', { query: { project, limit: ALL_ISH } }) - // There might be duplicates here, but that's fine; inclides() will early return on the first match + // There might be duplicates here, but that's fine; includes() will early return on the first match const allUnavailableDiskNames = [ ...(allDisks?.items.map((d) => d.name) || []), ...unavailableDiskNames, From af32dfe6c451ca739248451fd8c4259df1c6206f Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 14:28:04 -0800 Subject: [PATCH 04/10] Make sure boot disk can't be named the same as a disk being created in the same process; add test --- app/forms/disk-create.tsx | 4 +++- app/forms/instance-create.tsx | 20 ++++++++++++++++--- test/e2e/instance-create.e2e.ts | 34 +++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 157e0463ca..5d58b1c83c 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -79,7 +79,9 @@ export function CreateDiskSideModalForm({ const { project } = useProjectSelector() const { data: allDisks } = useApiQuery('diskList', { query: { project, limit: ALL_ISH } }) - // There might be duplicates here, but that's fine; includes() will early return on the first match + // There might be duplicates in allUnavailableDiskNames — as a disk from the allDisks list + // might be showing up in the unavailableDiskNames list (e.g. in the instance create flow) — but + // that's fine; includes() will early return on the first match const allUnavailableDiskNames = [ ...(allDisks?.items.map((d) => d.name) || []), ...unavailableDiskNames, diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 509b6dc698..7d4a9cdcb0 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -247,6 +247,12 @@ export function CreateInstanceForm() { } }, [createInstance.error]) + const existingDiskNames = allDisks.map((disk) => disk.name) + const otherDisks = useWatch({ control, name: 'otherDisks' }) + const namesOfDisksToBeCreated = otherDisks + .filter((disk) => disk.type === 'create') + .map((disk) => disk.name) + // additional form elements for projectImage and siloImage tabs const bootDiskSizeAndName = ( <> @@ -273,12 +279,17 @@ export function CreateInstanceForm() { required={false} control={control} disabled={isSubmitting} + validate={(name) => { + // don't allow the user to use an existing disk name for the boot disk's name + if ([...namesOfDisksToBeCreated, ...existingDiskNames].includes(name)) { + return 'Name is already in use' + } + }} /> ) - const otherDisks = useWatch({ control, name: 'otherDisks' }) - const unavailableDiskNames = otherDisks.map((disk) => disk.name) + const bootDiskName = useWatch({ control, name: 'bootDiskName' }) return ( <> @@ -567,7 +578,10 @@ export function CreateInstanceForm() { Authentication diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index c26de640f7..31b1a0b4f3 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -246,6 +246,40 @@ test('with disk name already taken', async ({ page }) => { await expectVisible(page, ['text=Disk name already exists']) }) +test('can’t create a disk with a name that collides with the boot disk name', async ({ + page, +}) => { + // Set up the instance and name the boot disk "disk-11" + await page.goto('/projects/mock-project/instances-new') + await page.fill('input[name=name]', 'another-instance') + await selectAProjectImage(page, 'image-1') + await page.fill('input[name=bootDiskName]', 'disk-11') + + // Attempt to create a disk with the same name + await page.getByRole('button', { name: 'Create new disk' }).click() + const dialog = page.getByRole('dialog') + await dialog.getByRole('textbox', { name: 'name' }).fill('disk-11') + await dialog.getByRole('button', { name: 'Create disk' }).click() + // Expect to see an error message + await expect(page.getByText('Name is already in use').first()).toBeVisible() + // Change the disk name to something else + await dialog.getByRole('textbox', { name: 'name' }).fill('disk-12') + await page.getByRole('button', { name: 'Create disk' }).click() + // The disk has been "created" (is in the list of Additional Disks) + await expectVisible(page, ['text=disk-12']) + // Create the instance + await page.getByRole('button', { name: 'Create instance' }).click() + await expect(page).toHaveURL('/projects/mock-project/instances/another-instance/storage') + + // Find the Boot Disk table and verify that disk-11 is there + const bootDiskTable = page.getByRole('table', { name: 'Boot disk' }) + await expect(bootDiskTable.getByRole('cell', { name: 'disk-11' })).toBeVisible() + + // Find the Other Disks table and verify that disk-12 is there + const otherDisksTable = page.getByRole('table', { name: 'Other disks' }) + await expect(otherDisksTable.getByRole('cell', { name: 'disk-12' })).toBeVisible() +}) + test('add ssh key from instance create form', async ({ page }) => { await page.goto('/projects/mock-project/instances-new') From 18de31d2de4eed71849807ce1360c01dbd5cdca7 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 14:49:42 -0800 Subject: [PATCH 05/10] small refactor --- app/forms/disk-create.tsx | 4 +--- test/e2e/instance-create.e2e.ts | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 5d58b1c83c..2269f461ba 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -79,9 +79,7 @@ export function CreateDiskSideModalForm({ const { project } = useProjectSelector() const { data: allDisks } = useApiQuery('diskList', { query: { project, limit: ALL_ISH } }) - // There might be duplicates in allUnavailableDiskNames — as a disk from the allDisks list - // might be showing up in the unavailableDiskNames list (e.g. in the instance create flow) — but - // that's fine; includes() will early return on the first match + // duplicates in allDisks and unavailableDiskNames is fine; includes() will early return on the first match const allUnavailableDiskNames = [ ...(allDisks?.items.map((d) => d.name) || []), ...unavailableDiskNames, diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 31b1a0b4f3..0fe1c7f402 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -261,10 +261,10 @@ test('can’t create a disk with a name that collides with the boot disk name', await dialog.getByRole('textbox', { name: 'name' }).fill('disk-11') await dialog.getByRole('button', { name: 'Create disk' }).click() // Expect to see an error message - await expect(page.getByText('Name is already in use').first()).toBeVisible() + await expect(dialog.getByText('Name is already in use')).toBeVisible() // Change the disk name to something else await dialog.getByRole('textbox', { name: 'name' }).fill('disk-12') - await page.getByRole('button', { name: 'Create disk' }).click() + await dialog.getByRole('button', { name: 'Create disk' }).click() // The disk has been "created" (is in the list of Additional Disks) await expectVisible(page, ['text=disk-12']) // Create the instance From a5c4dd56f0ddbbb4e39fd474d28de1128deb110e Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Tue, 12 Nov 2024 14:59:23 -0800 Subject: [PATCH 06/10] Update test --- test/e2e/instance-create.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 0fe1c7f402..23b819fc48 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -243,7 +243,7 @@ test('with disk name already taken', async ({ page }) => { await page.fill('input[name=bootDiskName]', 'disk-1') await page.getByRole('button', { name: 'Create instance' }).click() - await expectVisible(page, ['text=Disk name already exists']) + await expect(page.getByText('Name is already in use').first()).toBeVisible() }) test('can’t create a disk with a name that collides with the boot disk name', async ({ From ef05bb21be5424060fc806c773a2d4a761988d13 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 14 Nov 2024 11:17:58 -0800 Subject: [PATCH 07/10] let API handle disk-create where new name overlaps with existing disks --- app/forms/disk-create.tsx | 9 +-------- app/forms/instance-create.tsx | 11 +++++++---- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 2269f461ba..0651803b8f 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -36,7 +36,6 @@ import { FieldLabel } from '~/ui/lib/FieldLabel' import { Radio } from '~/ui/lib/Radio' import { RadioGroup } from '~/ui/lib/RadioGroup' import { Slash } from '~/ui/lib/Slash' -import { ALL_ISH } from '~/util/consts' import { toLocaleDateString } from '~/util/date' import { bytesToGiB, GiB } from '~/util/units' @@ -78,12 +77,6 @@ export function CreateDiskSideModalForm({ const navigate = useNavigate() const { project } = useProjectSelector() - const { data: allDisks } = useApiQuery('diskList', { query: { project, limit: ALL_ISH } }) - // duplicates in allDisks and unavailableDiskNames is fine; includes() will early return on the first match - const allUnavailableDiskNames = [ - ...(allDisks?.items.map((d) => d.name) || []), - ...unavailableDiskNames, - ] const createDisk = useApiMutation('diskCreate', { onSuccess(data) { @@ -146,7 +139,7 @@ export function CreateDiskSideModalForm({ name="name" control={form.control} validate={(name: string) => { - if (allUnavailableDiskNames.includes(name)) { + if (unavailableDiskNames.includes(name)) { return 'Name is already in use' } }} diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index 277c1df5cf..c08f01758a 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -583,10 +583,13 @@ export function CreateInstanceForm() { Authentication From 9590e69bc5b428f3bde406a6480c7dc96d536b28 Mon Sep 17 00:00:00 2001 From: Charlie Park Date: Thu, 14 Nov 2024 11:19:50 -0800 Subject: [PATCH 08/10] Move const declaration back to where it had been --- app/forms/disk-create.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/forms/disk-create.tsx b/app/forms/disk-create.tsx index 0651803b8f..7be48a4bb1 100644 --- a/app/forms/disk-create.tsx +++ b/app/forms/disk-create.tsx @@ -76,8 +76,6 @@ export function CreateDiskSideModalForm({ const queryClient = useApiQueryClient() const navigate = useNavigate() - const { project } = useProjectSelector() - const createDisk = useApiMutation('diskCreate', { onSuccess(data) { queryClient.invalidateQueries('diskList') @@ -88,6 +86,7 @@ export function CreateDiskSideModalForm({ }) const form = useForm({ defaultValues }) + const { project } = useProjectSelector() const projectImages = useApiQuery('imageList', { query: { project } }) const siloImages = useApiQuery('imageList', {}) From f75048c566050eb9e7e7645da95d0a62e4e42732 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Nov 2024 15:37:33 -0600 Subject: [PATCH 09/10] test that client-side created disks are disallowed --- test/e2e/instance-create.e2e.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/e2e/instance-create.e2e.ts b/test/e2e/instance-create.e2e.ts index 0742d4d613..4e0a5a6b81 100644 --- a/test/e2e/instance-create.e2e.ts +++ b/test/e2e/instance-create.e2e.ts @@ -551,7 +551,7 @@ test('create instance with additional disks', async ({ page }) => { await createForm.getByRole('button', { name: 'Create disk' }).click() await expect(createForm.getByText('Name is already in use')).toBeVisible() - // rename that failed disk + // rename the disk to one that's allowed await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1') await createForm.getByRole('button', { name: 'Create disk' }).click() @@ -559,6 +559,13 @@ test('create instance with additional disks', async ({ page }) => { await expect(disksTable.getByText('disk-6')).toBeHidden() await expectRowVisible(disksTable, { Name: 'new-disk-1', Type: 'create', Size: '5GiB' }) + // now that name is taken too, so disk create disallows it + await page.getByRole('button', { name: 'Create new disk' }).click() + await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1') + await createForm.getByRole('button', { name: 'Create disk' }).click() + await expect(createForm.getByText('Name is already in use')).toBeVisible() + await createForm.getByRole('button', { name: 'Cancel' }).click() + // Attach an existing disk await page.getByRole('button', { name: 'Attach existing disk' }).click() await selectOption(page, 'Disk name', 'disk-3') From 759f44bfa94d8dc5685dd968b522a05dcd6377ca Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 14 Nov 2024 15:44:14 -0600 Subject: [PATCH 10/10] tweak unavailable names list to save a couple lines --- app/forms/instance-create.tsx | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index c08f01758a..1cf6151e5c 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -252,11 +252,11 @@ export function CreateInstanceForm() { } }, [createInstance.error]) - const existingDiskNames = allDisks.map((disk) => disk.name) const otherDisks = useWatch({ control, name: 'otherDisks' }) - const namesOfDisksToBeCreated = otherDisks - .filter((disk) => disk.type === 'create') - .map((disk) => disk.name) + const unavailableDiskNames = [ + ...allDisks, // existing disks from the API + ...otherDisks.filter((disk) => disk.type === 'create'), // disks being created here + ].map((d) => d.name) // additional form elements for projectImage and siloImage tabs const bootDiskSizeAndName = ( @@ -286,7 +286,7 @@ export function CreateInstanceForm() { disabled={isSubmitting} validate={(name) => { // don't allow the user to use an existing disk name for the boot disk's name - if ([...namesOfDisksToBeCreated, ...existingDiskNames].includes(name)) { + if (unavailableDiskNames.includes(name)) { return 'Name is already in use' } }} @@ -585,11 +585,7 @@ export function CreateInstanceForm() { disabled={isSubmitting} // Don't allow the user to create a new disk with a name that matches other disk names (either the boot disk, // the names of disks that will be created and attached to this instance, or disks that already exist). - unavailableDiskNames={[ - bootDiskName, - ...namesOfDisksToBeCreated, - ...existingDiskNames, - ]} + unavailableDiskNames={[bootDiskName, ...unavailableDiskNames]} /> Authentication