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

improving how httpclients can be configured #4490

Merged
merged 4 commits into from Oct 18, 2022
Merged

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented Oct 11, 2022

Description

The first commit is to fix #4472 - refined withRequestConfig so that a new client is not created. This removes the need / ambiguity or exposing the factory via the client - which is needed for #4471. It also makes withRequestConfig much lighter weight as it no longer creates an entirely new client. On the downside it requires a lot of wiring and a new Inteceptor method so that it can pick up the modified, rather than the default Config. The 6.0 changes allowed the maxConcurrentRequests and maxConcurrentRequestsPerHost to actually work - but as far as I can tell those did not work in 5.x. I vote for removing those methods from the RequestConfig entirely. I'll update the changes with docs based upon whatever we agree upon.

The second commit fixes #4471 - it adds a KubernetesClientBuilder.withHttpClientBuilderConsumer that can be used to configure the Builder that is created. We still need the KubernetesClientBuilder to take a Factory as TokenRefreshInterceptor expects to use the newBuilder() method to create a "fresh" client instance - that logic could be refactored of course, but that's too much for right now.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@manusa manusa added this to the 6.2.0 milestone Oct 18, 2022
@manusa
Copy link
Member

manusa commented Oct 18, 2022

but as far as I can tell those did not work in 5.x. I vote for removing those methods from the RequestConfig entirely. I'll update the changes with docs based upon whatever we agree upon.

This seems fine, we can deprecate them and remove them in a subsequent release

@manusa manusa force-pushed the 4471 branch 2 times, most recently from d0abd6e to 9717fa9 Compare October 18, 2022 10:45
@manusa
Copy link
Member

manusa commented Oct 18, 2022

I think I broke the feature while rebasing :( sorry :(
I hope you still have your local copy @shawkins

@shawkins
Copy link
Contributor Author

Sure, let me see.

@shawkins
Copy link
Contributor Author

This seems fine, we can deprecate them and remove them in a subsequent release

The pr currently has them removed - the rationale is that since they are probably used through the builder, a user unfortunately won't know they are deprecated unless they read the release notes.

@manusa
Copy link
Member

manusa commented Oct 18, 2022

This seems fine, we can deprecate them and remove them in a subsequent release

The pr currently has them removed - the rationale is that since they are probably used through the builder, a user unfortunately won't know they are deprecated unless they read the release notes.

that works too

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug E 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

41.2% 41.2% Coverage
5.9% 5.9% Duplication

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx

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