Skip to content

fix: remove okhttp dependency #1793

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

Merged
merged 1 commit into from
Mar 10, 2023
Merged

fix: remove okhttp dependency #1793

merged 1 commit into from
Mar 10, 2023

Conversation

metacosm
Copy link
Collaborator

@metacosm metacosm commented Mar 2, 2023

No description provided.

@metacosm metacosm requested a review from csviri March 2, 2023 17:32
@metacosm metacosm self-assigned this Mar 2, 2023
@metacosm metacosm force-pushed the remove-okhttp branch 2 times, most recently from 0480f6b to 9918f6f Compare March 3, 2023 13:03
@metacosm metacosm marked this pull request as ready for review March 3, 2023 16:34
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

Just mad one comment. Also not sure if the main pom.xml should be reformatted, but otherwise LGTM


<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-httpclient-jdk</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not be this the vertx client?

Copy link
Collaborator Author

@metacosm metacosm Mar 8, 2023

Choose a reason for hiding this comment

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

As mentioned previously off-line, I would like to have tests with different clients to see if there is any impact

@@ -54,6 +54,11 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-httpclient-okhttp</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is okhttp in this module? Probably should be vertx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's test with all the different clients in integration tests in a different PR. In this PR, I wanted to have "some" coverage of the different http client implementations without getting into all possible combinations.

@@ -38,6 +38,10 @@
<groupId>io.javaoperatorsdk</groupId>
<artifactId>operator-framework</artifactId>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-httpclient-okhttp</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should not be just okhttp in one of these?

Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

38.8% 38.8% Coverage
0.0% 0.0% Duplication

@metacosm metacosm merged commit 902c8a5 into next Mar 10, 2023
@metacosm metacosm deleted the remove-okhttp branch March 10, 2023 15:20
csviri pushed a commit that referenced this pull request Mar 14, 2023
Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
csviri pushed a commit that referenced this pull request Mar 15, 2023
Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
metacosm added a commit that referenced this pull request Mar 22, 2023
Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
metacosm added a commit that referenced this pull request Mar 27, 2023
Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
csviri pushed a commit that referenced this pull request Mar 27, 2023
Also document how to use a different implementation instead of the
okhttp one which is the default one and currently, the one we recommend
to use. Vert.X implementation is also coming along nicely. JDK
implementation should work if you're using Java 17 and up, and will work
with 11 when some bug fixes land in 11.
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.

2 participants