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

Use DI for HttpClient.Builder #1250

Merged
merged 8 commits into from
Jan 27, 2025
Merged

Use DI for HttpClient.Builder #1250

merged 8 commits into from
Jan 27, 2025

Conversation

rfc2822
Copy link
Member

@rfc2822 rfc2822 commented Jan 19, 2025 •
edited
Loading

HttpClient.Builder was a bit unclean and didn't use DI.

This PR attempts to bring HttpClient.Builder to the current architecture pattern.

Also it removes support for the deprecated TLS 1.0 and 1.1 (#1249).

@rfc2822 rfc2822 added the refactoring Internal improvement of existing functions label Jan 19, 2025
@rfc2822 rfc2822 force-pushed the httpclient-builder-hilt branch 4 times, most recently from f5673aa to 628643d Compare January 19, 2025 21:26
@rfc2822 rfc2822 marked this pull request as ready for review January 19, 2025 21:30
@rfc2822 rfc2822 requested a review from sunkup January 19, 2025 21:30
@rfc2822
Copy link
Member Author

rfc2822 commented Jan 19, 2025

@sunkup Sorry I couldn't make it shorter ^^

Can you have a look?

@rfc2822 rfc2822 self-assigned this Jan 20, 2025
@rfc2822 rfc2822 mentioned this pull request Jan 20, 2025
Closed
@rfc2822 rfc2822 linked an issue Jan 20, 2025 that may be closed by this pull request
Turn off TLS 1.0 and 1.1 #1249
Closed
@rfc2822 rfc2822 force-pushed the httpclient-builder-hilt branch from 628643d to 18c3514 Compare January 25, 2025 18:49
sunkup
sunkup requested changes Jan 27, 2025
View reviewed changes
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

You are right this is a lot to review. Not sure whether I missed something, but at a glance all files look good to me. I don't really know enough about certificates to understand the extraction of
ClientCertKeyManager.kt, but that seems to work ok as well.

I restarted the CI tests, since they were aborted but seems like they are getting stuck at some point (testDoQueryChildren_updateTwoParentsSimultaneous ?), run forever and then are aborted? Locally that test fails as well, but I can't make out why.

@rfc2822
Copy link
Member Author

rfc2822 commented Jan 27, 2025

I restarted the CI tests, since they were aborted but seems like they are getting stuck at some point (testDoQueryChildren_updateTwoParentsSimultaneous ?), run forever and then are aborted? Locally that test fails as well, but I can't make out why.

I'll have a look

@rfc2822 rfc2822 requested a review from sunkup January 27, 2025 11:37
@rfc2822 rfc2822 merged commit 835689a into main-ose Jan 27, 2025
5 checks passed
@rfc2822 rfc2822 deleted the httpclient-builder-hilt branch January 27, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunkup sunkup sunkup approved these changes

Assignees

@rfc2822 rfc2822

Labels
refactoring Internal improvement of existing functions
Projects
DAVx⁵ Releases
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Turn off TLS 1.0 and 1.1
2 participants
@rfc2822 @sunkup