Skip to content
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.

Sign up for GitHub

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

Fix auto-updating of boot disk size when selected image is larger than current disk size; add tests #1829

Merged
merged 7 commits into from
Dec 7, 2023
Merged

Fix auto-updating of boot disk size when selected image is larger than current disk size; add tests #1829

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c250f54
Fix disk size auto-updating when image changes; add tests
charliepark Dec 2, 2023
107d226
Merge branch 'main' into 1823-disk-size-input-no-longer-auto-updates-…
charliepark Dec 5, 2023
4f2bec0
Merge branch 'main' into 1823-disk-size-input-no-longer-auto-updates-…
charliepark Dec 6, 2023
6885220
Refactor DiskSizeField so that it has both default validators and can…
charliepark Dec 6, 2023
74f5a22
Remove unnecessary comment
charliepark Dec 6, 2023
ca5abc2
Add validation on Disk Create for building off an image, that disk si…
charliepark Dec 7, 2023
093d59a
Refactor, plus use proper react-hook-form syntax
charliepark Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions app/components/form/fields/DiskSizeField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
*
* Copyright Oxide Computer Company
*/
import type { FieldPath, FieldPathByValue, FieldValues } from 'react-hook-form'
import type {
FieldPath,
FieldPathByValue,
FieldValues,
ValidateResult,
} from 'react-hook-form'

import { MAX_DISK_SIZE_GiB } from '@oxide/api'

Expand All @@ -17,12 +22,19 @@ interface DiskSizeProps<
TName extends FieldPath<TFieldValues>,
> extends TextFieldProps<TFieldValues, TName> {
minSize?: number
validate?(diskSizeGiB: number): ValidateResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more of a FYI about React Hook form, not a suggestion, because I can't bring myself to suggest something so much worse than what you have, and I don't think it's really more typesafe in practice. But if you hover on the validate prop, you can see it is using some helpers from RHF to look up the value of the field using the field name.

Screenshot 2023-12-06 at 5 00 41 PM

If you wanted to match that type on the prop, you could do this:

  validate?: Validate<PathValue<TFieldValues, TName>, TFieldValues>

This produces the same result as what you have (the argument is a number, of course) except it also has a second argument, which is all the form values together:

image

This could be handy in theory because you could validate one field value based on the value of another. But we're not doing that — well, we're doing it, but we're doing it at the call site so this field doesn't have to know about it. So we don't need it, and the resulting code is dramatically harder to understand, plus the second arg is required (lol) so it actually gets mad at you for leaving it out.

image

All this is to say, I've already typed this so I might as well shoot it into the ether, but it is not a suggestion to use this horrible type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's good to know, about, though! Thanks for writing that all out.

}

export function DiskSizeField<
TFieldValues extends FieldValues,
TName extends FieldPathByValue<TFieldValues, number>,
>({ required = true, name, minSize = 1, ...props }: DiskSizeProps<TFieldValues, TName>) {
>({
required = true,
name,
minSize = 1,
validate,
...props
}: DiskSizeProps<TFieldValues, TName>) {
return (
<NumberField
units="GiB"
Expand All @@ -32,12 +44,18 @@ export function DiskSizeField<
min={minSize}
max={MAX_DISK_SIZE_GiB}
validate={(diskSizeGiB) => {
// Run a number of default validators
if (Number.isNaN(diskSizeGiB)) {
return 'Disk size is required'
}
if (diskSizeGiB < minSize) {
return `Must be at least ${minSize} GiB`
}
if (diskSizeGiB > MAX_DISK_SIZE_GiB) {
return `Can be at most ${MAX_DISK_SIZE_GiB} GiB`
}
// Run any additional validators passed in from the callsite
return validate?.(diskSizeGiB)
}}
{...props}
/>
Expand Down
5 changes: 2 additions & 3 deletions app/components/form/fields/NumberField.tsx
View file
Open in desktop
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
62 changes: 36 additions & 26 deletions app/forms/instance-create.tsx
View file
Open in desktop
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 @@ -137,9 +136,11 @@ export function CreateInstanceForm() {

const imageInput = useWatch({ control: control, name: 'image' })
const image = allImages.find((i) => i.id === imageInput)
const imageSize = image?.size ? Math.ceil(image.size / GiB) : undefined

return (
<FullPageForm
submitDisabled={allImages.length ? undefined : 'Image required'}
id="create-instance-form"
form={form}
title="Create instance"
Expand All @@ -151,9 +152,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 +289,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'
}
Comment on lines +295 to +298
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice touch 👍

>
<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,15 +344,9 @@ 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`
validate={(diskSizeGiB: number) => {
if (imageSize && diskSizeGiB < imageSize) {
return `Must be as large as selected image (min. ${imageSize} GiB)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we had also added imageSize to the DiskSizeField on the disk create form last week, but it seems we did not. You could do it in this PR since it should only be couple of lines. A slightly tricky bit in that one is that there isn't always an image (and unlike here, that's not a pathological case, because you might not be creating the disk from an image).

}
}}
/>
Expand Down
37 changes: 37 additions & 0 deletions app/test/e2e/instance-create.e2e.ts
View file
Open in desktop
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'])
})

Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the test!

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
Loading