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

[wip] Update actix web to v4.0.0-beta.15 #677

Merged
merged 2 commits into from
Feb 6, 2022

Conversation

Jikstra
Copy link
Contributor

@Jikstra Jikstra commented Dec 28, 2021

Currently wip/draft as there's some things left to do:

src/config.rs Outdated
.with_safe_default_protocol_versions()
.unwrap()
.with_no_client_auth()
.with_single_cert(vec!(rustls::Certificate(cert_chain)), rustls::PrivateKey(keys.remove(0)))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did well with refactoring this code, but please carefully review as I have not too much clue about tls things.

Copy link
Contributor

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

Thanks for taking this forward.

src/main.rs Outdated Show resolved Hide resolved
src/listing.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
@Jikstra Jikstra changed the title [wip] Update actix web to v4.0.0-beta.16 [wip] Update actix web to v4.0.0-beta.15 Dec 28, 2021
@Jikstra
Copy link
Contributor Author

Jikstra commented Dec 28, 2021

Thanks for taking this forward.

Thanks for your initial pr :)

@Jikstra
Copy link
Contributor Author

Jikstra commented Dec 28, 2021

@aliemjay I think in your pr one test failed because of a regression in actix-web. To me it looked like the test was a bit weird anyways? Lot's of special chars if I remember correctly. Probably we can just kick out that test?

@aliemjay
Copy link
Contributor

Probably we can just kick out that test?

I don't think so. It is there for a reason and, if I remember, it fixed a reported issue. Anyway, it is on the plan for v4.

@aliemjay
Copy link
Contributor

Oh, I forgot to note that most of my PR changes are now merged because I split it into a different PR. You may want to resolve the conflicts, but I think it is cleaner to rebase the last commits.

@Jikstra
Copy link
Contributor Author

Jikstra commented Dec 28, 2021

Oh, I forgot to note that most of my PR changes are now merged because I split it into a different PR. You may want to resolve the conflicts, but I think it is cleaner to rebase the last commits.

I think I successfully rebased this pr :)

@Jikstra Jikstra force-pushed the update_actix_web branch 2 times, most recently from 303bb0c to 197a331 Compare December 28, 2021 02:20
Co-authored-by: Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>
@Jikstra
Copy link
Contributor Author

Jikstra commented Dec 28, 2021

Soooo, all looks good now besides this one failing test. @aliemjay could we somehow decode those chars on our own in miniserve or do we need to wait for actix_web to fix this?

@aliemjay
Copy link
Contributor

do we need to wait for actix_web to fix this?

I believe yes. There is nothing we can do in our part. I'll push for it and try to get it soon.

@aliemjay
Copy link
Contributor

aliemjay commented Jan 4, 2022

Actix-web beta.19 is now out with a fix to the regression.

@Jikstra
Copy link
Contributor Author

Jikstra commented Jan 4, 2022

Awesome!

@svenstaro
Copy link
Owner

Hey, how are we looking on the fix? The actix-web 4 rc is out so now might be a good time to get this merged.

@Jikstra
Copy link
Contributor Author

Jikstra commented Feb 4, 2022

I will not have time to work on this before mid of march, so feel free to just continue this pr whoever wants to :)

@svenstaro svenstaro merged commit caca691 into svenstaro:master Feb 6, 2022
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