Skip to content

Commit

Permalink
Fix disk size auto-updating when image changes; add tests
Browse files Browse the repository at this point in the history
  • Loading branch information
charliepark committed Dec 2, 2023
1 parent ed0ef62 commit c250f54
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 32 deletions.
15 changes: 14 additions & 1 deletion app/components/form/fields/DiskSizeField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,20 @@ interface DiskSizeProps<
TFieldValues extends FieldValues,
TName extends FieldPath<TFieldValues>,
> extends TextFieldProps<TFieldValues, TName> {
imageSize?: number
minSize?: number
}

export function DiskSizeField<
TFieldValues extends FieldValues,
TName extends FieldPathByValue<TFieldValues, number>,
>({ required = true, name, minSize = 1, ...props }: DiskSizeProps<TFieldValues, TName>) {
>({
required = true,
name,
imageSize,
minSize = 1,
...props
}: DiskSizeProps<TFieldValues, TName>) {
return (
<NumberField
units="GiB"
Expand All @@ -32,6 +39,12 @@ export function DiskSizeField<
min={minSize}
max={MAX_DISK_SIZE_GiB}
validate={(diskSizeGiB) => {
if (Number.isNaN(diskSizeGiB)) {
return 'Disk size is required'
}
if (imageSize && diskSizeGiB < imageSize) {
return `Must be as large as selected image (min. ${imageSize} GiB)`
}
if (diskSizeGiB < minSize) {
return `Must be at least ${minSize} GiB`
}
Expand Down
5 changes: 2 additions & 3 deletions app/components/form/fields/NumberField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export const NumberFieldInner = <
name={name}
control={control}
rules={{ required, validate }}
render={({ field: { value, ...fieldRest }, fieldState: { error } }) => {
render={({ field, fieldState: { error } }) => {
return (
<>
<UINumberField
Expand All @@ -87,8 +87,7 @@ export const NumberFieldInner = <
[`${id}-help-text`]: !!description,
})}
aria-describedby={description ? `${id}-label-tip` : undefined}
defaultValue={value}
{...fieldRest}
{...field}
/>
<ErrorMessage error={error} label={label} />
</>
Expand Down
61 changes: 33 additions & 28 deletions app/forms/instance-create.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
genName,
INSTANCE_MAX_CPU,
INSTANCE_MAX_RAM_GiB,
MAX_DISK_SIZE_GiB,
useApiMutation,
useApiQueryClient,
usePrefetchedApiQuery,
Expand Down Expand Up @@ -140,6 +139,7 @@ export function CreateInstanceForm() {

return (
<FullPageForm
submitDisabled={allImages.length ? undefined : 'Image required'}
id="create-instance-form"
form={form}
title="Create instance"
Expand All @@ -151,9 +151,10 @@ export function CreateInstanceForm() {
values.presetId === 'custom'
? { memory: values.memory, ncpus: values.ncpus }
: { memory: preset.memory, ncpus: preset.ncpus }

// we should also never have an image ID that's not in the list
const image = allImages.find((i) => values.image === i.id)
// There should always be an image present, because …
// - The form is disabled unless there are images available.
// - The form defaults to including at least one image.
invariant(image, 'Expected image to be defined')

const bootDiskName = values.bootDiskName || genName(values.name, image.name)
Expand Down Expand Up @@ -287,37 +288,51 @@ export function CreateInstanceForm() {
<FormDivider />

<Form.Heading id="boot-disk">Boot disk</Form.Heading>
<Tabs.Root id="boot-disk-tabs" className="full-width" defaultValue="project">
<Tabs.Root
id="boot-disk-tabs"
className="full-width"
// default to the project images tab if there are only project images
defaultValue={
projectImages.length > 0 && siloImages.length === 0 ? 'project' : 'silo'
}
>
<Tabs.List aria-describedby="boot-disk">
<Tabs.Trigger value="project">Project images</Tabs.Trigger>
<Tabs.Trigger value="silo">Silo images</Tabs.Trigger>
<Tabs.Trigger value="project">Project images</Tabs.Trigger>
</Tabs.List>
<Tabs.Content value="project" className="space-y-4">
{projectImages.length === 0 ? (
{allImages.length === 0 && (
<Message
className="mb-8 ml-10 max-w-lg"
variant="notice"
content="Images are required to create a boot disk."
/>
)}
<Tabs.Content value="silo" className="space-y-4">
{siloImages.length === 0 ? (
<div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
<EmptyMessage
icon={<Images16Icon />}
title="No project images found"
body="An image needs to be uploaded to be seen here"
buttonText="Upload image"
onClick={() => navigate(pb.projectImageNew(projectSelector))}
title="No silo images found"
body="Project images need to be promoted to be seen here"
/>
</div>
) : (
<ImageSelectField images={projectImages} control={control} />
<ImageSelectField images={siloImages} control={control} />
)}
</Tabs.Content>
<Tabs.Content value="silo" className="space-y-4">
{siloImages.length === 0 ? (
<Tabs.Content value="project" className="space-y-4">
{projectImages.length === 0 ? (
<div className="flex max-w-lg items-center justify-center rounded-lg border p-6 border-default">
<EmptyMessage
icon={<Images16Icon />}
title="No silo images found"
body="Project images need to be promoted to be seen here"
title="No project images found"
body="An image needs to be uploaded to be seen here"
buttonText="Upload image"
onClick={() => navigate(pb.projectImageNew(projectSelector))}
/>
</div>
) : (
<ImageSelectField images={siloImages} control={control} />
<ImageSelectField images={projectImages} control={control} />
)}
</Tabs.Content>
</Tabs.Root>
Expand All @@ -328,17 +343,7 @@ export function CreateInstanceForm() {
label="Disk size"
name="bootDiskSize"
control={control}
// Imitate API logic: only require that the disk is big enough to fit the image
validate={(diskSizeGiB) => {
if (!image) return true
if (diskSizeGiB < image.size / GiB) {
const minSize = Math.ceil(image.size / GiB)
return `Must be as large as selected image (min. ${minSize} GiB)`
}
if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
}
}}
imageSize={image?.size ? Math.ceil(image.size / GiB) : undefined}
/>
<NameField
name="bootDiskName"
Expand Down
37 changes: 37 additions & 0 deletions app/test/e2e/instance-create.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,43 @@ test('can create an instance with custom hardware', async ({ page }) => {
])
})

test('automatically updates disk size when larger image selected', async ({ page }) => {
await page.goto('/projects/mock-project/instances-new')

const instanceName = 'my-new-instance'
await page.fill('input[name=name]', instanceName)

// set the disk size larger than it needs to be, to verify it doesn't get reduced
const diskSizeInput = page.getByRole('textbox', { name: 'Disk size (GiB)' })
await diskSizeInput.fill('5')

// pick a disk image that's smaller than 5GiB (the first project image works [4GiB])
await page.getByRole('tab', { name: 'Project images' }).click()
await page.getByRole('button', { name: 'Image' }).click()
await page.getByRole('option', { name: images[0].name }).click()

// test that it still says 5, as that's larger than the given image
await expect(diskSizeInput).toHaveValue('5')

// pick a disk image that's larger than 5GiB (the third project image works [6GiB])
await page.getByRole('button', { name: 'Image' }).click()
await page.getByRole('option', { name: images[2].name }).click()

// test that it has been automatically increased to next-largest incremement of 10
await expect(diskSizeInput).toHaveValue('10')

// pick another image, just to verify that the diskSizeInput stays as it was
await page.getByRole('button', { name: 'Image' }).click()
await page.getByRole('option', { name: images[1].name }).click()
await expect(diskSizeInput).toHaveValue('10')

const submitButton = page.getByRole('button', { name: 'Create instance' })
await submitButton.click()

await expect(page).toHaveURL(`/projects/mock-project/instances/${instanceName}/storage`)
await expectVisible(page, [`h1:has-text("${instanceName}")`, 'text=10 GiB'])
})

test('with disk name already taken', async ({ page }) => {
await page.goto('/projects/mock-project/instances-new')
await page.fill('input[name=name]', 'my-instance')
Expand Down

0 comments on commit c250f54

Please sign in to comment.