Add oxide instance external-ip attach/detach
#499
Conversation
|
It looks like we're adding with regard to detach (for which the parameter is |
|
Thanks for taking a look!
I think we could do so, which give us some first-pass options:
That's a good point. We do need to add more fields to |
cli/src/cmd_instance.rs
Outdated
| Err(ref e @ oxide::Error::InvalidResponsePayload(ref r)) if r.is_decode() => { | ||
| // This is a minor hack to account for the case where a double | ||
| // ephemeral detach returns null -- the operation was a success, | ||
| // for idempotency, but we no longer know what the actual IP object | ||
| // referenced is/would be. | ||
| let formatted = format!("{r}"); | ||
| if formatted.contains("invalid type: null, expected struct ExternalIp") { | ||
| println!("success\n()") | ||
| } else { | ||
| println!("error\n{e:#?}") | ||
| } | ||
| } | ||
| Err(r) => { | ||
| println!("error\n{r:#?}") | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like I'm overlooking a way on the omicron side to avoid having to do this -- the return type there is Option'd, but this is not reflected in the output OpenAPI spec.
There was a problem hiding this comment.
You're missing something but it's not at all obvious. Top-level Option around a response does not work well. The good way to do this is to put it inside a non-optional struct:
struct MyResponseBody {
key: Option<Value>
}There was a problem hiding this comment.
As noted in omicron, I think the answer is likely to respond with an error if the entity we're trying to detach is not found.
FelixMcFelix
force-pushed
the
ip-attach
branch
from
84c2567 to
33b54a3
Compare
| @@ -268,7 +268,7 @@ impl RunnableCmd for CmdInstanceFromImage { | |||
| .expect("valid disk name"), | |||
| size: self.size.clone(), | |||
| }]) | |||
| .external_ips(vec![ExternalIpCreate::Ephemeral { pool_name: None }]) | |||
| .external_ips(vec![ExternalIpCreate::Ephemeral { pool: None }]) | |||
There was a problem hiding this comment.
is this now a required parameter?
There was a problem hiding this comment.
It's not a required parameter, this fixes a breaking field name change as we now take an Option<NameOrId>.
There was a problem hiding this comment.
That makes so much more sense. Sorry for the dumb question. Going too fast!
Relies on oxidecomputer/omicron#4694. This adds the autogenerated commands:
oxide floating-ip attach --floating-ip <floating-ip> --kind <kind> --parent <parent>oxide floating-ip detach --floating-ip <floating-ip>oxide instance external-ip attach-ephemeral --instance <instance>oxide instance external-ip detach-ephemeral --instance <instance>