From ce35f6a2a53b254dedc92176c900549aae580688 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 7 Oct 2024 13:50:14 -0500 Subject: [PATCH 1/2] refactor instanceCan and diskCan for better type safety --- app/api/util.spec.ts | 21 ++++++++++++- app/api/util.ts | 72 ++++++++++++++++++++++++-------------------- 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/app/api/util.spec.ts b/app/api/util.spec.ts index 1c3f97fb23..51fdf260ab 100644 --- a/app/api/util.spec.ts +++ b/app/api/util.spec.ts @@ -7,7 +7,7 @@ */ import { describe, expect, it, test } from 'vitest' -import { genName, parsePortRange, synthesizeData } from './util' +import { diskCan, genName, instanceCan, parsePortRange, synthesizeData } from './util' describe('parsePortRange', () => { describe('parses', () => { @@ -136,3 +136,22 @@ describe('synthesizeData', () => { ]) }) }) + +test('instanceCan', () => { + expect(instanceCan.start({ runState: 'running' })).toBe(false) + expect(instanceCan.start({ runState: 'stopped' })).toBe(true) + + // @ts-expect-error typechecker rejects actions that don't exist + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + instanceCan.abc +}) + +test('diskCan', () => { + expect(diskCan.delete({ state: { state: 'creating' } })).toBe(false) + expect(diskCan.delete({ state: { state: 'attached', instance: 'xyz' } })).toBe(false) + expect(diskCan.delete({ state: { state: 'detached' } })).toBe(true) + + // @ts-expect-error typechecker rejects actions that don't exist + // eslint-disable-next-line @typescript-eslint/no-unused-expressions + diskCan.abc +}) diff --git a/app/api/util.ts b/app/api/util.ts index d2a42750dd..cd09db3a56 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -89,47 +89,53 @@ export const genName = (...parts: [string, ...string[]]) => { ) } -const instanceActions: Record = { +// setting .states is a cute way to make it ergonomic to call the test function +// while also making the states available directly + +function makeInstanceTest(states: InstanceState[]) { + const test = (i: { runState: InstanceState }) => states.includes(i.runState) + test.states = states + return test +} + +// we used to define this by mapping actions directly to states and then running +// a mapValues on it, but that resulted in TS allowing any string as a key on +// instanceKey + +export const instanceCan = { // NoVmm maps to to Stopped: // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-model/src/instance_state.rs#L55 // https://github.com/oxidecomputer/omicron/blob/0496637/nexus/src/app/instance.rs#L2064 - start: ['stopped', 'failed'], + start: makeInstanceTest(['stopped', 'failed']), // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/instance.rs#L865 - delete: ['stopped', 'failed'], + delete: makeInstanceTest(['stopped', 'failed']), // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1030-L1043 - update: ['stopped', 'failed', 'creating'], + update: makeInstanceTest(['stopped', 'failed', 'creating']), // reboot and stop are kind of weird! // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L790-L798 // https://github.com/oxidecomputer/propolis/blob/b278193/bin/propolis-server/src/lib/vm/request_queue.rs // https://github.com/oxidecomputer/console/pull/2387#discussion_r1722368236 - reboot: ['running'], // technically rebooting allowed but too weird to say it + reboot: makeInstanceTest(['running']), // technically rebooting allowed but too weird to say i)t // stopping a failed disk: https://github.com/oxidecomputer/omicron/blob/f0b804818b898bebdb317ac2b000618944c02457/nexus/src/app/instance.rs#L818-L830 - stop: ['running', 'starting', 'rebooting', 'failed'], + stop: makeInstanceTest(['running', 'starting', 'rebooting', 'failed']), // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L323-L327 - detachDisk: ['creating', 'stopped', 'failed'], + detachDisk: makeInstanceTest(['creating', 'stopped', 'failed']), // only Creating and NoVmm // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L185-L188 - attachDisk: ['creating', 'stopped'], + attachDisk: makeInstanceTest(['creating', 'stopped']), + // primary nic: https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/network_interface.rs#L761-L765 // non-primary: https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/network_interface.rs#L806-L810 - updateNic: ['stopped'], + updateNic: makeInstanceTest(['stopped']), // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L1520-L1522 - serialConsole: ['running', 'rebooting', 'migrating', 'repairing'], -} - -// setting .states is a cute way to make it ergonomic to call the test function -// while also making the states available directly -export const instanceCan = R.mapValues(instanceActions, (states) => { - const test = (i: { runState: InstanceState }) => states.includes(i.runState) - test.states = states - return test -}) + serialConsole: makeInstanceTest(['running', 'rebooting', 'migrating', 'repairing']), +} export function instanceTransitioning({ runState }: Instance) { return ( @@ -140,30 +146,30 @@ export function instanceTransitioning({ runState }: Instance) { ) } -const diskActions: Record = { +function makeDiskTest(states: DiskState['state'][]) { + // only have to Pick because we want this to work for both Disk and + // Json, which we pass to it in the MSW handlers + const test = (d: Pick) => states.includes(d.state.state) + test.states = states + return test +} + +export const diskCan = { // this is a weird one because the list of states is dynamic and it includes // 'creating' in the unwind of the disk create saga, but does not include // 'creating' in the disk delete saga, which is what we care about // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/sagas/disk_delete.rs?plain=1#L110 - delete: ['detached', 'faulted'], + delete: makeDiskTest(['detached', 'faulted']), // TODO: link to API source. It's hard to determine from the saga code what the rule is here. - snapshot: ['attached', 'detached'], + snapshot: makeDiskTest(['attached', 'detached']), // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L173-L176 - attach: ['creating', 'detached'], + attach: makeDiskTest(['creating', 'detached']), // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L313-L314 - detach: ['attached'], + detach: makeDiskTest(['attached']), // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1077-L1081 - setAsBootDisk: ['attached'], + setAsBootDisk: makeDiskTest(['attached']), } -export const diskCan = R.mapValues(diskActions, (states) => { - // only have to Pick because we want this to work for both Disk and - // Json, which we pass to it in the MSW handlers - const test = (d: Pick) => states.includes(d.state.state) - test.states = states - return test -}) - /** Hard coded in the API, so we can hard code it here. */ export const FLEET_ID = '001de000-1334-4000-8000-000000000000' From e0206e0e7b860e097b6292d96e9a35042ea0b08b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 15 Oct 2024 14:31:11 -0500 Subject: [PATCH 2/2] ok well I did it --- app/api/util.ts | 74 +++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/app/api/util.ts b/app/api/util.ts index cd09db3a56..954cd77b0e 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -89,53 +89,47 @@ export const genName = (...parts: [string, ...string[]]) => { ) } -// setting .states is a cute way to make it ergonomic to call the test function -// while also making the states available directly - -function makeInstanceTest(states: InstanceState[]) { - const test = (i: { runState: InstanceState }) => states.includes(i.runState) - test.states = states - return test -} - -// we used to define this by mapping actions directly to states and then running -// a mapValues on it, but that resulted in TS allowing any string as a key on -// instanceKey - -export const instanceCan = { +const instanceActions = { // NoVmm maps to to Stopped: // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-model/src/instance_state.rs#L55 // https://github.com/oxidecomputer/omicron/blob/0496637/nexus/src/app/instance.rs#L2064 - start: makeInstanceTest(['stopped', 'failed']), + start: ['stopped', 'failed'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/instance.rs#L865 - delete: makeInstanceTest(['stopped', 'failed']), + delete: ['stopped', 'failed'], // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1030-L1043 - update: makeInstanceTest(['stopped', 'failed', 'creating']), + update: ['stopped', 'failed', 'creating'], // reboot and stop are kind of weird! // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L790-L798 // https://github.com/oxidecomputer/propolis/blob/b278193/bin/propolis-server/src/lib/vm/request_queue.rs // https://github.com/oxidecomputer/console/pull/2387#discussion_r1722368236 - reboot: makeInstanceTest(['running']), // technically rebooting allowed but too weird to say i)t + reboot: ['running'], // technically rebooting allowed but too weird to say it // stopping a failed disk: https://github.com/oxidecomputer/omicron/blob/f0b804818b898bebdb317ac2b000618944c02457/nexus/src/app/instance.rs#L818-L830 - stop: makeInstanceTest(['running', 'starting', 'rebooting', 'failed']), + stop: ['running', 'starting', 'rebooting', 'failed'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L323-L327 - detachDisk: makeInstanceTest(['creating', 'stopped', 'failed']), + detachDisk: ['creating', 'stopped', 'failed'], // only Creating and NoVmm // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L185-L188 - attachDisk: makeInstanceTest(['creating', 'stopped']), - + attachDisk: ['creating', 'stopped'], // primary nic: https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/network_interface.rs#L761-L765 // non-primary: https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/network_interface.rs#L806-L810 - updateNic: makeInstanceTest(['stopped']), + updateNic: ['stopped'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/instance.rs#L1520-L1522 + serialConsole: ['running', 'rebooting', 'migrating', 'repairing'], +} satisfies Record - serialConsole: makeInstanceTest(['running', 'rebooting', 'migrating', 'repairing']), -} +// setting .states is a cute way to make it ergonomic to call the test function +// while also making the states available directly + +export const instanceCan = R.mapValues(instanceActions, (states: InstanceState[]) => { + const test = (i: { runState: InstanceState }) => states.includes(i.runState) + test.states = states + return test +}) export function instanceTransitioning({ runState }: Instance) { return ( @@ -146,29 +140,29 @@ export function instanceTransitioning({ runState }: Instance) { ) } -function makeDiskTest(states: DiskState['state'][]) { - // only have to Pick because we want this to work for both Disk and - // Json, which we pass to it in the MSW handlers - const test = (d: Pick) => states.includes(d.state.state) - test.states = states - return test -} - -export const diskCan = { +const diskActions = { // this is a weird one because the list of states is dynamic and it includes // 'creating' in the unwind of the disk create saga, but does not include // 'creating' in the disk delete saga, which is what we care about // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/sagas/disk_delete.rs?plain=1#L110 - delete: makeDiskTest(['detached', 'faulted']), + delete: ['detached', 'faulted'], // TODO: link to API source. It's hard to determine from the saga code what the rule is here. - snapshot: makeDiskTest(['attached', 'detached']), + snapshot: ['attached', 'detached'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L173-L176 - attach: makeDiskTest(['creating', 'detached']), + attach: ['creating', 'detached'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L313-L314 - detach: makeDiskTest(['attached']), + detach: ['attached'], // https://github.com/oxidecomputer/omicron/blob/3093818/nexus/db-queries/src/db/datastore/instance.rs#L1077-L1081 - setAsBootDisk: makeDiskTest(['attached']), -} + setAsBootDisk: ['attached'], +} satisfies Record + +export const diskCan = R.mapValues(diskActions, (states: DiskState['state'][]) => { + // only have to Pick because we want this to work for both Disk and + // Json, which we pass to it in the MSW handlers + const test = (d: Pick) => states.includes(d.state.state) + test.states = states + return test +}) /** Hard coded in the API, so we can hard code it here. */ export const FLEET_ID = '001de000-1334-4000-8000-000000000000'