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 HttpClientBuilderCustomizer interface #890

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Add HttpClientBuilderCustomizer interface #890

merged 1 commit into from
Nov 3, 2023

Conversation

bananayong
Copy link
Contributor

@bananayong bananayong commented Sep 4, 2023

Hi, Olga.
I always thank for your efforts.

I would like to provide the ability for a user to customize settings, In addition to leveraging existing Spring Cloud settings.

In the previous version, ApacheHttpClientFactory interface existed. So I could customize HttpClientBuilder additionally.
But, in the current version, there is no method for HttpClientBuilder configurations.

Thank you!

httpClientProperties.getHc5().getConnectionRequestTimeoutUnit()))
.build());

HttpClientBuilderCustomizer customizer = customizerProvider.getIfAvailable();
Copy link
Member

Choose a reason for hiding this comment

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

Usually there is a list of this type of bean so there can be multiple contributions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. thx fast feedback!

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Merging #890 (a2b77bc) into main (89d30ad) will increase coverage by 0.00%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head a2b77bc differs from pull request most recent head 10073d2. Consider uploading reports for the commit 10073d2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #890   +/-   ##
=========================================
  Coverage     77.09%   77.10%           
- Complexity      582      583    +1     
=========================================
  Files            71       71           
  Lines          2296     2297    +1     
  Branches        308      308           
=========================================
+ Hits           1770     1771    +1     
  Misses          357      357           
  Partials        169      169           
Files Coverage Δ
...gn/clientconfig/HttpClient5FeignConfiguration.java 70.21% <100.00%> (+0.64%) ⬆️

@bananayong
Copy link
Contributor Author

I hope to ship this PR in the next release. 🙏

@OlgaMaciaszek
Copy link
Collaborator

Thanks for the PR, @bananayong . Could you please add a test and document the feature?

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek 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 the PR @bananayong. Please add a test and document the feature.

@OlgaMaciaszek OlgaMaciaszek added enhancement New feature or request and removed waiting-for-triage labels Sep 26, 2023
@bananayong
Copy link
Contributor Author

@OlgaMaciaszek
I added a test case and simple explanation for a HttpClientBuilderCustomizer interface.

Thank your feedback!

@bananayong
Copy link
Contributor Author

Is there any progress? @OlgaMaciaszek

@yong-kurly
Copy link

If I need more work, please let me know.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @bananayong, looks good. Please just update the license entries to -2023 and add your full name with @author tag to the javadocs of the classes you've changed - then it'll be ready to merge.

@bananayong
Copy link
Contributor Author

@OlgaMaciaszek Done. Please review 🙇

@OlgaMaciaszek OlgaMaciaszek added this to the 4.1.0 milestone Nov 2, 2023
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @bananayong - have noticed one more place lacking documentation. Please see the comment.

In addition to leveraging existing Spring Cloud settings, provide the ability for a user to customize settings.
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks, @bananayong. LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit 6efae03 into spring-cloud:main Nov 3, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants