Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Jan 30, 2026

Threw this together real quick to de-risk and get a sense of the vibes. API built from oxidecomputer/omicron#9731, which isn't merged yet and we're not sure if it will make it in for v18. This is built on top of #3005 because that has earlier API changes in it.

Fun questions

  • I put the read-only badge right in the name cell because it would be excessive to give it its own column, but really it doesn't feel great there either. Maybe if we had a read-only icon that you could hover on to get a label, it would work better in the name cell.
  • Read only: Yes in the side modal looks silly, also not as grouped together with snapshot ID as I would like.
  • Copy for the checkbox label (and possibly help text) — "Make disk read-only" is weird, as Ben points out
image image image

@vercel
Copy link

vercel bot commented Jan 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
console Ready Ready Preview Jan 30, 2026 10:33pm

Request Review


export const ReadOnlyBadge = () => (
<Badge color="neutral" className="relative">
read only
Copy link
Contributor

Choose a reason for hiding this comment

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

Such a silly nit, but I feel like even if it's made uppercase its good practice to case them properly

<SnapshotSelectField control={control} />
<div className="mt-2">
<CheckboxField name="diskBackend.diskSource.readOnly" control={control}>
Make disk read-only
Copy link
Contributor

Choose a reason for hiding this comment

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

"Make disk" is strange? Maybe just "Read-only"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put that in there tentatively because when it was just read-only, it wasn't clear enough that read-only refers to the resulting disk. So I think we need something more than read-only. Unless we just make the label "Read only" and we put help text in there that says "Instances won't be able to write to this disk" or something. I am also slightly torn about the hyphen. Generally I feel like hyphen is right for regular copy, but when it's just "Read-only" by itself it looks bad.

@benjaminleonard
Copy link
Contributor

Are there consequences to this action we want to communicate after the user has checked the box? Do we need to clarify that it cannot edited later?

@benjaminleonard
Copy link
Contributor

Scattergun thoughts contd:

  1. Can you use it for boot disks, or just additional ones?
    1a. If you can, how does cloud init work?
  2. Will it always just be for snapshots? Or images too?

@david-crespo
Copy link
Collaborator Author

david-crespo commented Jan 30, 2026

  1. Yes, can boot — sounds like this is the main use case
    a. Great question. @hawkw is checking. Edit: the cloud init volume is its own PCI device and should not be affected by the boot disk being read only.
  2. Images coming in like one hour lol

@david-crespo
Copy link
Collaborator Author

Further update on cloud-init: because cloud-init is fundamentally a program that runs in the guest OS and pulls data from the CIDATA volume, they just have to not do that (chat).

@hawkw
Copy link
Member

hawkw commented Jan 30, 2026

@benjaminleonard:

Are there consequences to this action we want to communicate after the user has checked the box? Do we need to clarify that it cannot edited later?

I'd like to add some stuff to the docs about read-only disks, perhaps the checkbox should have a link to there?

@david-crespo:

  1. Images coming in like one hour lol

As of oxidecomputer/omicron@4c3bd6b, it should also be an option for DiskSource::Image (slightly less than an hour lol). :)

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.

4 participants