oxidecomputer / dropshot

expose REST APIs from a Rust program
Apache License 2.0
843 stars 74 forks source link

upgrade to hyper v1 #1028

Closed seanmonstar closed 1 week ago

seanmonstar commented 4 months ago

This defines a dropshot::Body type that is used for both requests and responses, in place of the now-gone hyper::Body.

Most of the changes are just renames. The interesting files (at least) are:

jclulow commented 4 months ago

I currently make fairly extensive use of:

Body::wrap_stream(tokio_stream::wrappers::ReceiverStream::new(rx))

Will that still work with the new code?

seanmonstar commented 4 months ago

It'd be pretty straight forward to add such a constructor to dropshot::Body.

ahl commented 3 months ago

@seanmonstar this is looking great. I would like @davepacheco to take a pass before we merge it, but, @davepacheco, feel free to interpret that with whatever degree of rigor you consider appropriate.

davepacheco commented 3 months ago

Sorry I'm late to this one. I haven't had a chance to look yet. If there are breaking changes here, as it sounds like there are, then I'd like to get the changelog update in this PR. Our general approach is to provide super clear instructions for people to understand how to know if they're affected by a breaking change and exactly what changes to make, as concretely as we can. There are some examples here, from release 0.9.0.

@ahl also mentioned a design doc? That might help review?

davepacheco commented 3 months ago

I forgot to add: for breaking changes, I usually like to do a test update of one or more of our important consumers. We've found a lot of issues by doing that. Have we converted any consumers? Ideally it'd be great to convert Omicron's consumers -- is that too much to ask?

ahl commented 3 months ago

@ahl also mentioned a design doc? That might help review?

I followed up with dave privately with the document. We'll want to make sure it's recorded in this PR once we have appropriate ownership and permissions.

seanmonstar commented 3 months ago

I believe I've addressed all the feedback. I added some breaking changes notes to the changelog. Upgrading Omicron is currently out of scope for me.

davepacheco commented 3 months ago

I think the last items here are:

ahl commented 3 months ago

@seanmonstar it looks like Cargo.lock needs an upodate

ahl commented 3 months ago

@seanmonstar tests are failing

ahl commented 3 months ago

@seanmonstar looks like windows tests are hung?

ahl commented 3 months ago

Eyyyy! Passing tests! I'm working on merging this into omicron and its vassal repos; on vacation right now, but hope to shake out any kinks in a week or so.

ahl commented 1 week ago

WOOT