Skip to content
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.

Sign up for GitHub

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

Jump to bottom

External IPs endpoint returns struct with items Vec #1495

Merged
merged 1 commit into from
Jul 26, 2022
Merged

External IPs endpoint returns struct with items Vec #1495

merged 1 commit into from
Jul 26, 2022

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Jul 25, 2022 •
edited
Loading

Ran into this on console because the TS client generator doesn't handle this case well. It could handle it better, but this should still return a struct.

david-crespo
david-crespo commented Jul 25, 2022
View reviewed changes
nexus/src/external_api/views.rs Outdated
Self { items }
}
}

Copy link
Contributor Author

@david-crespo david-crespo Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice and generic, though there are so few of these unpaginated things that I wonder if it makes sense to instead do one-offs with an appropriately named key, like

struct ExternalIps {
  ips: Vec<ExternalIp>,
}

like in #1164. Either way we should standardize on one approach or the other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm inclined to have all list types be formatted the same just for ease of implementation. It reduces the delta between a paginated endpoint and a non-paginated endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case maybe I should call this Results to go with ResultsPage...

david-crespo
david-crespo commented Jul 25, 2022
View reviewed changes
openapi/nexus.json Outdated
@@ -7261,6 +7257,21 @@
}
]
},
"ExternalIpList": {
"description": "Wrapper for unpaginated lists of items",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew. Here's a reason to give each one its own struct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW ResultsPage has a similar issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… that we could certainly use our cleverness to improve!!

@ahl
Copy link
Contributor

ahl commented Jul 25, 2022

Returning unbounded (or intrinsically bounded) arrays seems fraught. I would suggest that we navigate all such interfaces to a paginated interface. Failing that, we can at least pick a response type forward-compatible with a paginated interface. In this case for example I’d suggest we return something named ExternalIpPage with items but no next_page field.we could even just use ResultPage and always construct it with a None next_page

@david-crespo
Copy link
Contributor Author

Makes sense. I was hoping to make it clear in the spec that the endpoint always gives you everything, but I can see how that's not future-proof and probably of limited usefulness anyway. On the console, at least, we can see that the endpoint doesn't take a limit param and in any case we know it's giving us everything.

@david-crespo
Copy link
Contributor Author

david-crespo commented Jul 25, 2022 •
edited
Loading

I'll wait until #1478 is in to merge this. That has enough conflicts to worry about as it is. I can do my console work against this branch anyway.

@david-crespo david-crespo mentioned this pull request Jul 25, 2022
Merged
@david-crespo david-crespo enabled auto-merge (squash) July 26, 2022 16:59
@david-crespo david-crespo merged commit 6d8d6a4 into main Jul 26, 2022
@david-crespo david-crespo deleted the external-ips-response branch July 26, 2022 18:18
@david-crespo david-crespo mentioned this pull request Aug 23, 2022
Closed
leftwo pushed a commit that referenced this pull request Oct 25, 2024
Update Crucible and Propolis
b981fcb 
Crucible changes
Add test and fix for replay race condition (#1519)
Fix clippy warnings (#1517)
Add edition to `crucible-workspace-hack` (#1516)
Split out Downstairs-specific stats (#1511)
Move remaining `GuestWork` functionality into `Downstairs` (#1510)
Track both jobs and bytes in each IO state (#1507)
Fix new `rustc` and `clippy` warnings (#1509)
Remove IOP/BW limits (for now) (#1506)
Move `GuestBlockRes` into `DownstairsIO` (#1502)
Update actions/checkout digest to eef6144 (#1499)
Update Rust crate hyper-staticfile to 0.10.1 (#1411)
Turn off test-up-2region-encrypted.sh (#1504)
Add `IOop::Barrier` (#1494)
Fix IPv6 addresses in `crutest` (#1503)
Add region set options to more tests. (#1496)
Simplify `CompleteJobs` (#1493)
Removed ignored CI jobs (#1497)
Minor cleanups to `print_last_completed` (#1501)
Remove remaining `Arc<Volume>` instances (#1500)
Add `VolumeBuilder` type (#1492)
remove old unused scripts (#1495)
More multiple region support. (#1484)
Simplify matches (#1490)
Move complete job tracker to a helper object (#1489)
Expand summary and add documentation references to the README. (#1486)
Remove `GuestWorkId` (2/2) (#1482)
Remove `JobId` from `DownstairsIO` (1/2) (#1481)
Remove unused `#[derive(..)]` (#1483)
Update more tests to use dsc (#1480)
Crutest now Volume only (#1479)

Propolis changes
manually impl Deserialize for PciPath for validation purposes (#801)
phd: gate OS-specific tests, make others more OS-agnostic (#799)
lib: log vCPU diagnostics on triple fault and for some unhandled exit types (#795)
add marker trait to help check safety of guest memory reads (#794)
clippy fixes for 1.82 (#796)
lib: move cpuid::Set to cpuid_utils; prevent semantic subleaf conflicts (#782)
PHD: write efivars in one go (#786)
PHD: support guest-initiated reboot (#785)
server: accept CPUID values in instance specs and plumb them to bhyve (#780)
PHD: allow patched Crucible dependencies (#778)
server: add a first-class error type to machine init (#777)
PciPath to Bdf conversion is infallible; prove it and refactor (#774)
instance spec rework: flatten InstanceSpecV0 (#767)
Make PUT /instance/state 503 when waiting to init
Less anxiety-inducing `Vm::{get, state_watcher}`
@leftwo leftwo mentioned this pull request Oct 25, 2024
Merged
leftwo added a commit that referenced this pull request Oct 30, 2024
@leftwo
Update Crucible and Propolis (#6932)
9c4a217 
Crucible changes
Add test and fix for replay race condition (#1519) Fix clippy warnings
(#1517)
Add edition to `crucible-workspace-hack` (#1516)
Split out Downstairs-specific stats (#1511)
Move remaining `GuestWork` functionality into `Downstairs` (#1510) Track
both jobs and bytes in each IO state (#1507) Fix new `rustc` and
`clippy` warnings (#1509)
Remove IOP/BW limits (for now) (#1506)
Move `GuestBlockRes` into `DownstairsIO` (#1502)
Update actions/checkout digest to eef6144 (#1499)
Update Rust crate hyper-staticfile to 0.10.1 (#1411) Turn off
test-up-2region-encrypted.sh (#1504)
Add `IOop::Barrier` (#1494)
Fix IPv6 addresses in `crutest` (#1503)
Add region set options to more tests. (#1496)
Simplify `CompleteJobs` (#1493)
Removed ignored CI jobs (#1497)
Minor cleanups to `print_last_completed` (#1501)
Remove remaining `Arc<Volume>` instances (#1500)
Add `VolumeBuilder` type (#1492)
remove old unused scripts (#1495)
More multiple region support. (#1484)
Simplify matches (#1490)
Move complete job tracker to a helper object (#1489) Expand summary and
add documentation references to the README. (#1486) Remove `GuestWorkId`
(2/2) (#1482)
Remove `JobId` from `DownstairsIO` (1/2) (#1481)
Remove unused `#[derive(..)]` (#1483)
Update more tests to use dsc (#1480)
Crutest now Volume only (#1479)

Propolis changes
manually impl Deserialize for PciPath for validation purposes (#801)
phd: gate OS-specific tests, make others more OS-agnostic (#799) lib:
log vCPU diagnostics on triple fault and for some unhandled exit types
(#795) add marker trait to help check safety of guest memory reads
(#794) clippy fixes for 1.82 (#796)
lib: move cpuid::Set to cpuid_utils; prevent semantic subleaf conflicts
(#782) PHD: write efivars in one go (#786)
PHD: support guest-initiated reboot (#785)
server: accept CPUID values in instance specs and plumb them to bhyve
(#780) PHD: allow patched Crucible dependencies (#778)
server: add a first-class error type to machine init (#777) PciPath to
Bdf conversion is infallible; prove it and refactor (#774) instance spec
rework: flatten InstanceSpecV0 (#767) Make PUT /instance/state 503 when
waiting to init
Less anxiety-inducing `Vm::{get, state_watcher}`

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zephraph zephraph zephraph left review comments

@ahl ahl ahl left review comments

@bnaecker bnaecker Awaiting requested review from bnaecker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@david-crespo @ahl @zephraph