-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement the determinations of RFD 285 #1329
Conversation
@@ -597,13 +596,15 @@ async fn silo_saml_idp_fetch( | |||
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await | |||
} | |||
|
|||
// TODO: no DELETE for identity providers? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davepacheco thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great! It makes me so happy to see these changes, thanks for pushing for semantic naming! 🎉
disk_create /organizations/{organization_name}/projects/{project_name}/disks | ||
disk_delete /organizations/{organization_name}/projects/{project_name}/disks/{disk_name} | ||
disk_list /organizations/{organization_name}/projects/{project_name}/disks | ||
disk_view /organizations/{organization_name}/projects/{project_name}/disks/{disk_name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression we were going to change all *_view
to *_get
? Unless I missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that it necessarily matters one way or the other, but GitHub's CLI uses view
and edit
. Regardless, it's all an upgrade to what we have today, ha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karencfv we had agreed on edit
-> update
but were 50-50 on view
vs. get
. I guess I slightly prefer get
since we might as well match the http method when it's natural to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually took the deliberate step not to match the HTTP method because I thought it might be confusing if some matched while others did not. That said: not a strong preference so if someone wants to make a case for one vs. the other that would be clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can definitely see that side too. It makes you assume things about the semantics that we may not want to imply. Or maybe we actually do because we want these to be spec compliant gets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this one as well 😅, but I think I'm leaning more towards the use of get
. It's a tad more consistent with what people will be used to seeing in SDKs generally. That said, while get
appears everywhere, there are also weird inconsistencies everywhere. Like for example, AWS uses some weird combination of "sometimes get
, sometimes describe
". Both Azure and GCP have the more consistent APIs/SDKs and VMware is all over the place. So , I guess sticking to view
isn't a huge deal as long as we don't deviate.
TLDR: I have slight preference for get
but not a huge deal if we stick with view
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go with _view
. I think of all the things to change in the future, s/_view/_get/
is going to be pretty easy in API/SDK/CLI/etc.
image_create /organizations/{organization_name}/projects/{project_name}/images | ||
image_delete /organizations/{organization_name}/projects/{project_name}/images/{image_name} | ||
image_list /organizations/{organization_name}/projects/{project_name}/images | ||
image_view /organizations/{organization_name}/projects/{project_name}/images/{image_name} | ||
|
||
API operations found with tag "images:global" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe unrelated to this PR, but why is the tag "images:global" and not "global_images"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah; let's set that aside? I agree though that it's ... odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a single tag for fleet-wide operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds really good!
Heya @ahl, I was just trying out this fancy new nexus.json file with oxide.rs and the CLI and it turns out the CLI broke in many more unexpected ways than just the method names 😅. Since the testing isn't complete in both CLI and SDKs. What do you think about merging this PR after the demo? I'm totally OK with breaking all the things after that date. Would it be too annoying to do that? @zephraph @david-crespo what do you think? |
This is a big customer-facing improvement across the board besides that, so it’s probably worth spending a couple of hours to see if maybe it’s less of a disaster in the client libs than it seems at first. How far off is the progenitor branch of Oxide.rs? Presumably it has fewer issues? |
I wish we could just push through with this too, but... I'm not sure we can. Without a larger team effort to tackle this migration @karencfv is right. There's a lot that would break.
We've got a lot of conditional logic around naming that makes the SDKs pretty fragile to changes like this. |
I already spent quite a bit more than a couple of hours on this unfortunately 😅 Like Justin says a lot would break 😢 There are two major pain points regarding the Rust SDK/CLI
Also, The work with progenitor is quite far from working with the CLI yet, and will not be able to work with the current CLI generator at all. |
There's also the point to consider that given the CLI will change, is it a good use of time to spend that much time updating it? |
Ok, that is persuasive. Seems like we should do the Progenitor stuff first so this change is less painful. On the Go SDK side since there’s no progenitor maybe we could at least reduce the amount of custom renaming logic first. |
Yep! Adam and I are working on the progenitor stuff now (from both sides of the fence 😄 ). I'm giving this top priority. The Go SDK will wait a bit as the Terraform provider is in demo condition anyway as there are no update endpoints for disks and instances |
The latest on the demo is that it may not be for another month. My hope for the demo was to have the API trending towards the state we want it to be in. I'm not clear on when the API/CLI/SDKs may be in the hands of early users, but I expect it will follow close on the heels of the demo. After remote access will come our own robust internal use, documentation, automation, etc. And then product launch. We will continue to expand and evolve the API. My hope was that by changing its trajectory now there would be less to tidy up later. That is to say: we follow the examples we see so if we can dominate the API with examples we want rather than examples we don't we will follow the positive examples. In short, I think we need to find a path forward. Here's a simple kludge: I can extract from this PR a one-to-one list of To be clear, there are going to be aspects of this that suck and it's going to be brittle. But I think the alternative may be to incur more change and more risk later in the product development lifecycle. I'd suggest interested parties meet tomorrow afternoon PT/morning NZ to discuss. |
Happy to catch up in my morning, but just want to comment that I'm perfectly fine with:
I'd really hate to stop this work from moving along, and this seems like a reasonable temporary solution while the new SDK/CLI is up and running :) |
@ahl, yep, that sounds like a solid path forward. |
Here is the list of renames (old new). I'm going to wait for a final 👍 from @karencfv this afternoon before merging.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ahl !! That list will keep the CLI alive until we have the new SDK
Added a new package, crucible-dtrace that pulls from buildomat a package that contains a set of DTrace scripts. These scripts are extracted into the global zone at /opt/oxide/crucible_dtrace/ Update Crucible to latest includes these updates: Clean up dependency checking, fixing space leak (#1372) Make a DTrace package (#1367) Use a single context in all messages (#1363) Remove `DownstairsWork`, because it's redundant (#1371) Remove `WorkState`, because it's implicit (#1370) Do work immediately upon receipt of a job, if possible (#1366) Move 'do work for one job' into a helper function (#1365) Remove `DownstairsWork` from map when handling it (#1361) Using `block_in_place` for IO operations (#1357) update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369) Parameterize more tests (#1364) Misc cleanup, remove sqlite references. (#1360) Fix `Extent::close` docstring (#1359) Make many `Region` functions synchronous (#1356) Remove `Workstate::Done` (unused) (#1355) Return a sorted `VecDeque` directly (#1354) Combine `proc_frame` and `do_work_for` (#1351) Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support arbitrary Volumes during replace compare (#1349) Remove the SQLite backend (#1352) Add a custom timeout for buildomat tests (#1344) Move `proc_frame` into `ActiveConnection` (#1348) Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into ConnectionState (#1340) Make `ConnectionState` an enum type (#1339) Parameterize `test_repair.sh` directories (#1345) Remove `Arc<Mutex<Downstairs>>` (#1338) Send message to Downstairs directly (#1336) Consolidate `on_disconnected` and `remove_connection` (#1333) Move disconnect logic to the Downstairs (#1332) Remove invalid DTrace probes. (#1335) Fix outdated comments (#1331) Use message passing when a new connection starts (#1330) Move cancellation into Downstairs, using a token to kill IO tasks (#1329) Make the Downstairs own per-connection state (#1328) Move remaining local state into a `struct ConnectionState` (#1327) Consolidate negotiation + IO operations into one loop (#1322) Allow replacement of a target in a read_only_parent (#1281) Do all IO through IO tasks (#1321) Make `reqwest_client` only present if it's used (#1326) Move negotiation into Downstairs as well (#1320) Update Rust crate clap to v4.5.4 (#1301) Reuse a reqwest client when creating Nexus clients (#1317) Reuse a reqwest client when creating repair client (#1324) Add % to keep buildomat happy (#1323) Downstairs task cleanup (#1313) Update crutest replace test, and mismatch printing. (#1314) Added more DTrace scripts. (#1309) Update Rust crate async-trait to 0.1.80 (#1298)
Update Crucible and Propolis to the latest Added a new package, crucible-dtrace that pulls from buildomat a package that contains a set of DTrace scripts. These scripts are extracted into the global zone at /opt/oxide/crucible_dtrace/ Crucible latest includes these updates: Clean up dependency checking, fixing space leak (#1372) Make a DTrace package (#1367) Use a single context in all messages (#1363) Remove `DownstairsWork`, because it's redundant (#1371) Remove `WorkState`, because it's implicit (#1370) Do work immediately upon receipt of a job, if possible (#1366) Move 'do work for one job' into a helper function (#1365) Remove `DownstairsWork` from map when handling it (#1361) Using `block_in_place` for IO operations (#1357) update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369) Parameterize more tests (#1364) Misc cleanup, remove sqlite references. (#1360) Fix `Extent::close` docstring (#1359) Make many `Region` functions synchronous (#1356) Remove `Workstate::Done` (unused) (#1355) Return a sorted `VecDeque` directly (#1354) Combine `proc_frame` and `do_work_for` (#1351) Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support arbitrary Volumes during replace compare (#1349) Remove the SQLite backend (#1352) Add a custom timeout for buildomat tests (#1344) Move `proc_frame` into `ActiveConnection` (#1348) Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into ConnectionState (#1340) Make `ConnectionState` an enum type (#1339) Parameterize `test_repair.sh` directories (#1345) Remove `Arc<Mutex<Downstairs>>` (#1338) Send message to Downstairs directly (#1336) Consolidate `on_disconnected` and `remove_connection` (#1333) Move disconnect logic to the Downstairs (#1332) Remove invalid DTrace probes. (#1335) Fix outdated comments (#1331) Use message passing when a new connection starts (#1330) Move cancellation into Downstairs, using a token to kill IO tasks (#1329) Make the Downstairs own per-connection state (#1328) Move remaining local state into a `struct ConnectionState` (#1327) Consolidate negotiation + IO operations into one loop (#1322) Allow replacement of a target in a read_only_parent (#1281) Do all IO through IO tasks (#1321) Make `reqwest_client` only present if it's used (#1326) Move negotiation into Downstairs as well (#1320) Update Rust crate clap to v4.5.4 (#1301) Reuse a reqwest client when creating Nexus clients (#1317) Reuse a reqwest client when creating repair client (#1324) Add % to keep buildomat happy (#1323) Downstairs task cleanup (#1313) Update crutest replace test, and mismatch printing. (#1314) Added more DTrace scripts. (#1309) Update Rust crate async-trait to 0.1.80 (#1298) Propolis just has this one update: Allow boot order config in propolis-standalone --------- Co-authored-by: Alan Hanson <alan@oxide.computer>
This implements the renames described in 285 to the best of my ability. I would appreciate a careful review of the names chosen with an eye towards:
Imagine that these names will translate verbatim into the CLI.
The
nexus_tags.txt
file may be the most concise way to consider this.