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.

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

Add multi part #716

Merged
merged 12 commits into from
Sep 15, 2023
Merged

Add multi part #716

merged 12 commits into from
Sep 15, 2023

Conversation

jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Jun 30, 2023

This adds support for multipart/form-data. Thanks! Let me know if you want anything to be changed but it came out super clean, better than I expected. Added tests as well :)

ping @ahl @davepacheco

@ahl
Copy link
Collaborator

ahl commented Jul 3, 2023

thanks, @jessfraz--might take us a little bit as we're in crunch time over here; apologies in advance for any delay

@jessfraz
Copy link
Contributor Author

jessfraz commented Jul 5, 2023

@ahl understandable! If it helps I figured you all would likely need this for a blob storage api or if you allow customers to upload their own isos to boot in vms :)

Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
Signed-off-by: Jess Frazelle <github@jessfraz.com>
@adamchalmers
Copy link
Contributor

Merge conflicts fixed!

The test for multipart didn't actually get a second part of the body, it just got the first one.
The test now concatenates all body parts, so it should check some boundary conditions.

According to `cargo llvm-cov` this increases test coverage of body.rs by 2 percent.
@ahl
Copy link
Collaborator

ahl commented Sep 14, 2023

@adamchalmers are you folks on the same page with regard to your comments above?

@adamchalmers
Copy link
Contributor

Yeah, I updated the tests to increase the coverage. Not really sure if we keep the Copyright Oxide header on test_multipart.rs but I also don't really care! Jess and I spoke last night and we're on the same page.

@jessfraz
Copy link
Contributor Author

LGTM thanks!

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

cool; thanks for this

@ahl ahl merged commit c7a5e43 into oxidecomputer:main Sep 15, 2023
11 checks passed
@adamchalmers
Copy link
Contributor

<3 Thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants