Add detach button for ephemeral ip #2285
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Looks good! I don’t think it should say the IP twice. Agree about doing attach in a separate PR. It should be really simple — all you can do is pick the pool. |
|
|
Let's make sure the title and button text match. We have two options:
|
|
|
Nitpick, but I have a slight preference for "Confirm detach floating IP" even though it's grammatically dubious. I think of it as an abbreviation for "Confirm (you want to) detach (the) floating IP". It fits a pattern we follow elsewhere: 
|
|
I vaguely remember that for the detach floating IP menu button, we went with non-destructive styling because the floating IP continues to exist after being detached. In this case, the ephemeral IP is no longer allocated and there's no way to guarantee you can get it back, so it is destructive. I wonder if there's a way to communicate that here. I think making this one red and the floating IP one not would be kind of weird, and it also wouldn't be sufficient to explain what's going on. I can't think of a great wording, but maybe a second sentence in this popup would help, like "Ephemeral IPs are deallocated when they are detached." Deallocated is not a user-meaningful term, really. It would be nice if we had a way to convey that that are Gone in some stronger sense, but not that strong, because the IP might well still be available in the pool. 
We could also maybe add a sentence to each confirmation? Like on floating IP "The floating IP will still be available to be reattached to this or another instance." and on ephemeral "Unlike floating IPs, ephemeral IPs are deallocated when detached, which means they may not remain available." Too many words, but that would be the gist. |
| // Detach the ephemeral IP | ||
| await clickRowAction(page, 'ephemeral', 'Detach') | ||
| await page.getByRole('button', { name: 'Confirm' }).click() | ||
| await expect(externalIpTable.getByRole('cell', { name: 'ephemeral' })).toBeHidden() |
There was a problem hiding this comment.
add an assert before clicking the row action that this same selector is visible. The clickRowAction implicitly tests for that, but I like to use the same selector for before-and-after checks.
mock-api/msw/handlers.ts
Outdated
| // https://github.com/oxidecomputer/omicron/blob/d52aad0/nexus/src/app/sagas/instance_ip_detach.rs#L79-L82 | ||
| const ip = db.ephemeralIps.find((eip) => eip.instance_id === instance.id) | ||
| if (!ip) throw notFoundErr('ephemeral IP') | ||
| if (!ip) throw notFoundErr('Could not find an ephemeral IP') |
There was a problem hiding this comment.
You had it right the first time. See this line:
Line 25 in c0f9f5e
I think if you wanted to be really fancy you could put the instance name or ID in there: "ephemeral IP for instance ${path.instance}"
oxidecomputer/console@a06d852...34c648b * [34c648b5](oxidecomputer/console@34c648b5) bump omicron to latest main (vpc router endpoints) * [44ea9cbe](oxidecomputer/console@44ea9cbe) fix casing in floating IP create toast * [b4b1103e](oxidecomputer/console@b4b1103e) oxidecomputer/console#2249 * [dcec501d](oxidecomputer/console@dcec501d) oxidecomputer/console#2286 * [ed4d92ec](oxidecomputer/console@ed4d92ec) oxidecomputer/console#2285
oxidecomputer/console@a06d852...34c648b * [34c648b5](oxidecomputer/console@34c648b5) bump omicron to latest main (vpc router endpoints) * [44ea9cbe](oxidecomputer/console@44ea9cbe) fix casing in floating IP create toast * [b4b1103e](oxidecomputer/console@b4b1103e) oxidecomputer/console#2249 * [dcec501d](oxidecomputer/console@dcec501d) oxidecomputer/console#2286 * [ed4d92ec](oxidecomputer/console@ed4d92ec) oxidecomputer/console#2285
Fixes #2205
This adds a
Detachbutton for ephemeral IPs, with a confirmation:Now that we can detach ephemeral IPs, we also need an empty state for that table, so this PR also adds that:
I believe we don't currently have a way to attach an ephemeral IP to an instance in the UI, but I suspect that's something we'll tackle separately.