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

Bump kubernetes-client-bom from 6.3 to 6.4 #30480

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

manusa
Copy link
Contributor

@manusa manusa commented Jan 19, 2023

Kubernetes Client 6.4.0 6.4.1 was just released: https://github.com/fabric8io/kubernetes-client/releases/tag/v6.4.1

This release of the client includes the new Vert.x HttpClient implementation. This PR includes changes in the pom.xml files to make use of this client instead of the default OkHttp implementation. If everything goes well, this should fix #29520

  • Added an enforcer rule to ensure OkHttp that the kubernetes-httpclient-okhttp module is not mistakenly propagated, however, I'm not sure this is the right place. I can't directly block OkHttp since it is also used (server-side) in the MockServer.
  • Modified the kubernetes-extension (creates kubernetes et al manifests and deploys them) to create and close KubernetesClient instances and avoid the leakages mentioned in some of the PR comments
  • Modified the kubernetes-client-extension to implement an HttpClient.Factory that reuses the global Vert.x instance (uses CDI to retrieve it, maybe doesn't work, waiting for tests)

/cc @metacosm @maxandersen @shawkins @gastaldi @geoand

Fix #30558

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

That's awesome! You can also add the exclusion to the bom/application pom.xml

@gastaldi
Copy link
Contributor

That's awesome! You can also add the exclusion to the bom/application pom.xml

Ah but we only import the kubernetes-bom there, nevermind

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Awesome!

As a followup, I will try to remove okhttp from the BOM as it shouldn't be a dependency anymore.

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

@gastaldi gastaldi dismissed their stale review January 19, 2023 14:18

Guillaume will followup the Okhttp/Okio removal in a separate PR

@gsmet
Copy link
Member

gsmet commented Jan 19, 2023

Yeah I think it's better to do it in a separate PR in case it breaks something and we have to revert it.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Very glad to see this!

@manusa manusa marked this pull request as draft January 19, 2023 15:30
@gsmet
Copy link
Member

gsmet commented Jan 19, 2023

An example of the enforcer failures:

[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (enforce) @ quarkus-kubernetes-client-internal ---
[18537](https://github.com/quarkusio/quarkus/actions/runs/3959243390/jobs/6781818473#step:10:18538)
Warning:  Rule 3: org.apache.maven.plugins.enforcer.ExternalRules failed with message:
[18538](https://github.com/quarkusio/quarkus/actions/runs/3959243390/jobs/6781818473#step:10:18539)
Found Banned Dependency: com.squareup.okhttp3:okhttp:jar:3.14.9
[18539](https://github.com/quarkusio/quarkus/actions/runs/3959243390/jobs/6781818473#step:10:18540)
Use 'mvn dependency:tree' to locate the source of the banned dependencies.

@manusa manusa marked this pull request as ready for review January 20, 2023 06:14
@manusa manusa marked this pull request as draft January 20, 2023 06:30
@manusa manusa force-pushed the deps/kubernetes-client branch 3 times, most recently from 941ebf9 to 239ae37 Compare January 20, 2023 15:00
@manusa
Copy link
Contributor Author

manusa commented Jan 20, 2023

It seems that we might need to bump Dekorate along with the client.

dekorateio/dekorate#1131

@geoand
Copy link
Contributor

geoand commented Jan 23, 2023

Including @Sgitario for awareness

@Sgitario
Copy link
Contributor

Including @Sgitario for awareness

PR to bump dekorate: #30533

Sgitario added a commit to Sgitario/quarkus that referenced this pull request Jan 24, 2023
@manusa manusa force-pushed the deps/kubernetes-client branch 3 times, most recently from 14d8677 to d5ac934 Compare January 24, 2023 14:36
@gsmet
Copy link
Member

gsmet commented Feb 1, 2023

Don't worry about the Gradle tests failing, it's unrelated to your patch.

@manusa
Copy link
Contributor Author

manusa commented Feb 1, 2023

Don't worry about the Gradle tests failing, it's unrelated to your patch.

Thx. Yes, I discussed internally with Georgios. I've rebased after the merge of #30758.

Hoping this time we get a completely green report 🤞 .

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 1, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand merged commit c68cf1b into quarkusio:main Feb 1, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Feb 1, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Feb 1, 2023
geoand added a commit to geoand/quarkus that referenced this pull request Feb 2, 2023
With quarkusio#30480, we no longer depend on okhttp,
so there is no reason to keep it in the BOM anymore
geoand added a commit to geoand/quarkus that referenced this pull request Feb 2, 2023
@geoand geoand mentioned this pull request Feb 2, 2023
geoand added a commit to geoand/quarkus that referenced this pull request Feb 2, 2023
@manusa manusa deleted the deps/kubernetes-client branch February 6, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes extension build error decouple from okhttp(3)
8 participants