Skip to content
Merged
3 changes: 3 additions & 0 deletions app/components/form/fields/DisksTableField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ export type DiskTableItem =
export function DisksTableField({
control,
disabled,
unavailableDiskNames,
}: {
control: Control<InstanceCreateInput>
disabled: boolean
unavailableDiskNames: string[]
}) {
const [showDiskCreate, setShowDiskCreate] = useState(false)
const [showDiskAttach, setShowDiskAttach] = useState(false)
Expand Down Expand Up @@ -108,6 +110,7 @@ export function DisksTableField({
onChange([...items, { type: 'create', ...values }])
setShowDiskCreate(false)
}}
unavailableDiskNames={unavailableDiskNames}
onDismiss={() => setShowDiskCreate(false)}
/>
)}
Expand Down
12 changes: 11 additions & 1 deletion app/forms/disk-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ type CreateSideModalFormProps = {
*/
onDismiss: (navigate: NavigateFunction) => void
onSuccess?: (disk: Disk) => void
unavailableDiskNames?: string[]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do like this a lot better without adding disks from the API in here. I'd add a comment here on the prop saying this is for client-side use-cases like the form embedded in instance create, where submitting the form does not tell you whether it's taken.

}

export function CreateDiskSideModalForm({
onSubmit,
onSuccess,
onDismiss,
unavailableDiskNames = [],
}: CreateSideModalFormProps) {
const queryClient = useApiQueryClient()
const navigate = useNavigate()
Expand Down Expand Up @@ -132,7 +134,15 @@ export function CreateDiskSideModalForm({
loading={createDisk.isPending}
submitError={createDisk.error}
>
<NameField name="name" control={form.control} />
<NameField
name="name"
control={form.control}
validate={(name: string) => {
if (unavailableDiskNames.includes(name)) {
return 'Name is already in use'
}
}}
/>
<DescriptionField name="description" control={form.control} />
<FormDivider />
<DiskSourceField
Expand Down
22 changes: 21 additions & 1 deletion app/forms/instance-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,12 @@ export function CreateInstanceForm() {
}
}, [createInstance.error])

const otherDisks = useWatch({ control, name: 'otherDisks' })
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 = (
<>
Expand All @@ -278,10 +284,18 @@ 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 (unavailableDiskNames.includes(name)) {
return 'Name is already in use'
}
}}
/>
</>
)

const bootDiskName = useWatch({ control, name: 'bootDiskName' })

return (
<>
<PageHeader>
Expand Down Expand Up @@ -566,7 +580,13 @@ export function CreateInstanceForm() {
</Tabs.Root>
<FormDivider />
<Form.Heading id="additional-disks">Additional disks</Form.Heading>
<DisksTableField control={control} disabled={isSubmitting} />
<DisksTableField
control={control}
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, ...unavailableDiskNames]}
/>
<FormDivider />
<Form.Heading id="authentication">Authentication</Form.Heading>
<SshKeysField control={control} isSubmitting={isSubmitting} />
Expand Down
53 changes: 51 additions & 2 deletions test/e2e/instance-create.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,41 @@ 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 ({
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(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 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
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 }) => {
Expand Down Expand Up @@ -510,13 +544,28 @@ 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 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()

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' })

// 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')
Expand Down