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 a flag for forcing a specific http version #161

Merged
merged 18 commits into from
Oct 14, 2021
Merged

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Jun 30, 2021

Resolves #68

@svenstaro
Copy link

What's left to do here? Why is it a draft?

@ducaale
Copy link
Owner Author

ducaale commented Jul 5, 2021

@svenstaro we are waiting for the next version of reqwest so we can make use of the newly added http1_only() option.

@ducaale
Copy link
Owner Author

ducaale commented Jul 11, 2021

Note to self: Remove the following from README.md

HTTP/2 cannot be disabled. (#68)

Edit: Done

@ducaale
Copy link
Owner Author

ducaale commented Jul 31, 2021

Note to self: Remove connection-related headers when --http-version=2 is used. See #132.

Also, update to_curl.rs.

Edit: Done

src/cli.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
@ducaale
Copy link
Owner Author

ducaale commented Oct 11, 2021

Three tests are currently failing due to expired certs. I will disable them until https://self-signed.badssl.com/ starts working again.

By the way, are there any HTTP version test servers similar to https://badssl.com?

Edit: The issue has been resolved so I will re-enable the tests that I've disabled.

@ducaale ducaale marked this pull request as ready for review October 11, 2021 16:34
@ducaale ducaale requested a review from blyxxyz October 13, 2021 04:44
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good!

src/cli.rs Outdated
Comment on lines 954 to 957
"0.9" | "3" => Err(Error::with_description(
&format!("http version {:?} is not supported", version),
ErrorKind::InvalidValue,
)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, possible_versions makes this unreachable. We could get rid of that and display the list of versions manually, which would also let us hide the 1 alias. Or we could just remove these manual error messages.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Since manually listing the versions meant they wouldn't get highlighted, I opted to just remove the manual error messages.

.assert()
.success()
.stdout(predicates::str::contains("GET / HTTP/1.0"))
.stdout(predicates::str::contains("HTTP/1.0 200 OK"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some servers respond to a 1.0 request with a 1.1 response, like httpbin.org and nginx. So this could break in the future, though I don't expect it. Maybe worth a comment?

@ducaale ducaale merged commit 806af98 into develop Oct 14, 2021
@ducaale ducaale deleted the force-http-version branch October 14, 2021 08:15
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