Cleanup list uninitialized sled API #4698
Conversation
* Rename `uninitialized_sled_list` to `sled_list_uninitialized` * Add fake pagination to this API endpoint Fixes part of #4607
|
Gentle ping @ahl @david-crespo @zephraph |
nexus/tests/output/nexus_tags.txt
Outdated
| @@ -125,12 +125,12 @@ rack_list GET /v1/system/hardware/racks | |||
| rack_view GET /v1/system/hardware/racks/{rack_id} | |||
| sled_instance_list GET /v1/system/hardware/sleds/{sled_id}/instances | |||
| sled_list GET /v1/system/hardware/sleds | |||
| sled_list_uninitialized GET /v1/system/hardware/uninitialized-sleds | |||
There was a problem hiding this comment.
Should we changed the endpoint to be /v1/system/hardware/sleds-uninitialized? This follows a similar pattern where in cases we may have wanted the API to be /sleds/uninitialized but can't because of naming conflicts, we use - instead.
There was a problem hiding this comment.
If that's the consensus, I can go ahead and make this change.
There was a problem hiding this comment.
I agree with Justin. This pattern is not obvious in the API because where it really shows up is the console, e.g., /projects vs. /projects-new.
There are a few endpoints in the API that are similar, like /v1/system/silo-quotas and /v1/vpc-subnets, but both of those are hierarchical noun-noun (parent-child) pairs so not quite the same thing. But yes, the basic principle is to line up the names so they're obviously related:
/sleds
/sleds-uninitialized
|
Thanks for the review and tips @zephraph @david-crespo! This was my first new nexus endpoint, so I'm not surprised I ran into something like this. |
| tags = ["system/hardware"] | ||
| }] | ||
| async fn uninitialized_sled_list( | ||
| async fn sled_list_uninitialized( | ||
| rqctx: RequestContext<Arc<ServerContext>>, |
There was a problem hiding this comment.
this is insufficient to make this into a paginated endpoint. We also need the Query<..> even though we'll ignore it.
There was a problem hiding this comment.
Sorry about that and thanks for the tip! I will update it in a follow up.
There was a problem hiding this comment.
I'm sorry I didn't get this comment in in time!
There was a problem hiding this comment.
Fixed in #4720
| "5XX": { | ||
| "$ref": "#/components/responses/Error" | ||
| } | ||
| } |
There was a problem hiding this comment.
we'd expect to see "x-dropshot-paginated" in here
There was a problem hiding this comment.
x-dropshot-pagination shows up as of #4720
uninitialized_sled_listtosled_list_uninitializedFixes part of #4607