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 1 commit
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
15 changes: 14 additions & 1 deletion app/components/form/fields/DiskSizeField.tsx
View file
Open in desktop
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)`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing we could do here is just update the min input threshold. We'd probably want to include an info icon with some details to indicate why the user can't go below the threshold though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that’s why we went with two thresholds, the message saying it’s due to the image is pretty helpful. See also #1824

if (diskSizeGiB < minSize) {
return `Must be at least ${minSize} GiB`
}
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
61 changes: 33 additions & 28 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 @@ -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'
}
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,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}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure that we should should push the validation logic down into the DiskSizeField component. Disks, as a concept, aren't strictly speaking tied to images. Certainly in this case the disk size is semi-dependent on the image size, but that doesn't impact other situations in which a disk size may be specified.

We'll either continue pushing more context down into the DiskSizeField component or have to find some other way of composing the logic. I think my preference, for now, is to localize the validation logic to where it's used (similar to what we had here). If we need that logic to be more portable perhaps we could just have some reusable helper we could spread into the usage?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, even though the logic is currently the same between the two spots, we could model it as a shared validate function that’s passed in in both spots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored this after chatting with @david-crespo … several of the validations are standard and will be needed for the DiskSizeField regardless of where we use it. But we also want to have the option of adding one-off validators. So we added an optional validate prop that we can pass in when calling the component; any validations passed in via that prop will get called after the standard validator functions are evaluated.

/>
<NameField
name="bootDiskName"
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