Skip to content

Conversation

@hawkw
Copy link
Member

@hawkw hawkw commented Jan 31, 2026

@ahl rightly points out that the common case for the disk_create APIs is to create read-write disks, and it seems like kind of a drag to require all calls to create a read-write disk from an image or snapshot to include a "read_only": false field in the JSON body. Thus, this commit adds #[serde(default)] so that disk source params without a "read_only" are the same as "read_only": false.

Unfortunately, we didn't catch this until after #9731 merged, so fixing it the proper way requires a whole new API version. Since the conversion between the Rust params types in nexus-types is trivial, it might have been possible to bend the rules a bit here and just tweak the previously-generated API version to make the field nullable --- clients operating with the slightly-older version of that version would still always be accepted since they would always send the field. But, bending the rules makes me scared I'm gonna get in trouble, so I did it the proper way, at the expense of a whole lot of code that does basically nothing.

@ahl rightly [points out][1] that the common case for the `disk_create`
APIs is to create read-write disks, and it seems unfortunate to require
all calls to create a read-write disk from an image or snapshot to
require a `"read_only": false` field in the JSON body. Thus, this commit
adds `#[serde(default)]` so that disk source params _without_ a
`"read_only"` are the same as `"read_only": false`.

Unfortunately, we didn't catch this until _after_ #9731 merged, so
fixing it the proper way requires a whole new API version. Since the
conversion between the Rust params types in `nexus-types` is trivial, it
might have been possible to bend the rules a bit here and just tweak the
previously-generated API version to make the field nullable --- clients
operating with the slightly-older version of that version would still
always be accepted since they would _always_ send the field. But,
bending the rules makes me scared I'm gonna get in trouble, so I did it
the proper way, at the expense of a whole lot of code that does
basically nothing.
@hawkw hawkw requested review from ahl and jmpesp January 31, 2026 17:57
@hawkw hawkw changed the title [nexus] Accept DiskSource::{Inmage, Snapshot}s without read_only [nexus] Accept DiskSource::{Image, Snapshot}s without read_only Jan 31, 2026
@hawkw hawkw requested a review from david-crespo January 31, 2026 18:01
@hawkw hawkw enabled auto-merge (squash) January 31, 2026 18:01
Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Can’t believe the conversions. In theory it could be fine to just overwrite the old version since nobody is running the previous version. It does feel completely ridiculous to have to do this.

@david-crespo
Copy link
Contributor

Confirmed it has the desired effect on the TS client:

--- a/2026013001.0.0-1c5abd/Api.ts
+++ b/2026013100.0.0-c09e48/Api.ts
@@ -1920,7 +1920,7 @@
   /** Create a disk from a disk snapshot */
   | {
       /** If `true`, the disk created from this snapshot will be read-only. */
-      readOnly: boolean;
+      readOnly?: boolean;
       snapshotId: string;
       type: "snapshot";
     }
@@ -1928,7 +1928,7 @@
   | {
       imageId: string;
       /** If `true`, the disk created from this image will be read-only. */
-      readOnly: boolean;
+      readOnly?: boolean;
       type: "image";
     }
   /** Create a blank disk that will accept bulk writes or pull blocks from an external source. */
@@ -7484,7 +7484,7 @@
    * Pulled from info.version in the OpenAPI schema. Sent in the
    * `api-version` header on all requests.
    */
-  apiVersion = "2026013001.0.0";
+  apiVersion = "2026013100.0.0";
 
   constructor({ host = "", baseParams = {}, token }: ApiConfig = {}) {
     this.host = host;

@hawkw hawkw merged commit e051ce7 into main Jan 31, 2026
16 checks passed
@hawkw hawkw deleted the eliza/nullable-readonly branch January 31, 2026 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants