Initial snapshots support #1204
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| if (diskErr) { | ||
| return res(diskErr) | ||
| } |
There was a problem hiding this comment.
Not important for our implementation but I wonder if the real API 404s or 400s when it can't find the disk.
There was a problem hiding this comment.
Unsure. It's just added in the chain of lookups so I presume it would be consistent with any other lookup failure.
| disks: (params: PP.Project) => `${pb.project(params)}/disks`, | ||
| vpcNew: (params: PP.Project) => `${pb.project(params)}/vpcs-new`, | ||
|
|
||
| snapshotNew: (params: PP.Project) => `${pb.project(params)}/snapshots-new`, |
app/forms/snapshot-create.tsx
Outdated
| > | ||
| <NameField id="snapshot-name" /> | ||
| <DescriptionField id="snapshot-description" /> | ||
| <NameField id="snapshot-disk" name="disk" label="Disk" /> |
There was a problem hiding this comment.
Why not a picker? There could be too many, and you wouldn't want to have some disk missing from the list and it'd be impossible to choose it. Makes me wonder if we need this form at all. What if we just have all snapshot creation done through the disk view? That makes some sense to me and avoids this problem entirely.
There was a problem hiding this comment.
You also avoid having to handle the case where the disk doesn't exist and we 404 on that. Though I guess technically it could still happen if the disk disappears right before we try to snapshot it.
There was a problem hiding this comment.
Picker would be the right experience, I was just trying to do a first pass. I think the form should exist because it's an explicitly discoverable avenue of creation whereas snapshotting a disk from the disks page is hidden away behind more actions.
I'll add the picker.
| }) | ||
| }, | ||
| disabled: disk.state.state !== 'attached', | ||
| }, |
There was a problem hiding this comment.
This button is awesome. It feels a little abrupt to me — what I expected to happen was it would pull up a side modal with the disk name pre-populated and let me pick a name (with the generated default also pre-populated).
There was a problem hiding this comment.
The abruptness is why I mentioned we need a better feedback mechanism.
There was a problem hiding this comment.
Really there are two issues, I think. In practice the snapshotting will not be instant (you should probably add a couple of seconds delay to the mock endpoint), so there's the feedback on completion, but I also think there's a general "ok what is happening here, what is being created, what is it called, where does it end up?" that might not be answered by the completion confirmation. Or at least, even if we do give some of that info at completion time, and make it easy, for example, to click through to wherever the snapshot lives, I think the user wants some of that info before they click the button so they can feel confident it's what they want to do.
app/forms/snapshot-create.tsx
Outdated
| return ( | ||
| disks?.items | ||
| .filter((disk) => disk.state.state === 'attached') | ||
| .map((disk) => ({ value: disk.name, label: disk.name, limit: 1000 })) || [] |
There was a problem hiding this comment.
yeah I think 1000 will be ok for now 😬
There was a problem hiding this comment.
lmao, not sure why I added it to the map though. Whew. That's... that's something.
| const { data: disks } = useApiQuery('diskList', { orgName, projectName }) | ||
| return ( | ||
| disks?.items | ||
| .filter((disk) => disk.state.state === 'attached') |
There was a problem hiding this comment.
Seems like the API will do it whether it's attached or not. Confirming with James.
There was a problem hiding this comment.
just kidding! snapshotting unattached disks is unimplemented
There was a problem hiding this comment.
Yeah, I checked with him about this yesterday. Another thing that could fail here is that I believe the instance actually has to be running. That's a much more sophisticated query though, so I just left it out.
There was a problem hiding this comment.
Yeah we should not do that. Maybe we need a snapshottable disks endpoint (or query param filter on the disks endpoint 🧐).
Wires up the ability to create and delete snapshots. A snapshot can either be created from the snapshots page or from the disk list page via the table action menu. Right now the
diskinput via snapshot creation is just a text input.