[FLINK-36332] Missed webhook okhttp reference.

[SamBarker]

What is the purpose of the change

There is still a stray reference to OkHTTP via the webhook.

Brief change log

(for example:)

• Fully exclude OkHttp

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): no
• The public API, i.e., is any changes to the CustomResourceDescriptors: no
• Core observer or reconciler logic that is regularly executed: no

Documentation

• Does this pull request introduce a new feature? no
• If yes, how is the feature documented? not applicable

I'd also like to back port this to release-1.10 if possible.

[tinaselenge]

LGTM :)

[k-wall]

Activation issue

I think there is a misunderstand about the way that Maven profiles are activated. Maven profile can be activated by system properties, not maven properties. The two concepts are separate. So if the intent is that build activates okhttp by default, that's not happening.

You can show this against main

mvn   help:all-profiles  | grep http
Profile Id: depend-on-okhttp4 (Active: false , Source: pom)

mvn   help:all-profiles -Dfabric8.httpclient.impl=okhttp | grep http
  Profile Id: depend-on-okhttp4 (Active: true , Source: pom)

I think depend-on-okhttp4 should be activated by default. If a user wants a alternative client, they should set the fabric8.httpclient.impl property and deactivate the profile.

com.squareup.okhttp3:logging-interceptor dependency

I notice that logging-interceptor is included as a dependency even if a client other than okhttp is selected.
I am running this against your PR branch.

mvn dependency:tree  -Dfabric8.httpclient.impl=jdk | grep com.squareup.okhttp3
[INFO] |  |  +- com.squareup.okhttp3:mockwebserver:jar:3.12.12:test
[INFO] |  |  |  +- com.squareup.okhttp3:okhttp:jar:3.12.12:test
[INFO] |     |     \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:test
[INFO] +- com.squareup.okhttp3:mockwebserver:jar:4.12.0:test
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:4.12.0:test
[INFO] \- com.squareup.okhttp3:mockwebserver:jar:4.12.0:test
[INFO]    +- com.squareup.okhttp3:okhttp:jar:4.12.0:test

I think logging-interceptor should probably be excluded.

Build pulls in okhttp3 modules at different version

I notice mockwebserver is being pulled in at 3.12.12 and 4.12.0. Whilst this is a test dependency but this could lead to surprising behaviour.

mvn dependency:tree  -Dfabric8.httpclient.impl=okhttp | grep com.squareup.okhttp3
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] |  |  +- com.squareup.okhttp3:mockwebserver:jar:3.12.12:test
[INFO] |     |     \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:test
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] |  \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:compile
[INFO] +- com.squareup.okhttp3:mockwebserver:jar:4.12.0:test
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] |  |  \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:provided
[INFO] +- com.squareup.okhttp3:mockwebserver:jar:4.12.0:test
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] |  |  \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:provided
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] |  |  \- com.squareup.okhttp3:logging-interceptor:jar:3.12.12:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile
[INFO] \- com.squareup.okhttp3:okhttp:jar:4.12.0:compile

I think a dependencyManagement should be applied:

[SamBarker]

Thanks @k-wall

I think depend-on-okhttp4 should be activated by default. If a user wants a alternative client, they should set the fabric8.httpclient.impl property and deactivate the profile.

Grr that is annoying distinction between properties that I missed. I'd been testing by toggling the -D flag when building with maven.

I'll have a look at the mockwebserver inconsistency as that was something I'd seen initially and thought I'd fixed.

[SamBarker]

I've added eede213 to address the issues highlighted by @k-wall

mvn help:effective-pom -pl :flink-kubernetes-operator -Doutput=/tmp/default-pom.xml
mvn help:effective-pom -pl :flink-kubernetes-operator -P\!depend-on-okhttp4 -Dfabric8.httpclient.impl=jdk -Doutput=/tmp/jdk-pom.xml
diff /tmp/*-pom.xml
4c4
< <!-- Generated by Maven Help Plugin on 2024-10-23T13:21:57+13:00            -->
---
> <!-- Generated by Maven Help Plugin on 2024-10-23T13:23:54+13:00            -->
551c551
<       <artifactId>kubernetes-httpclient-okhttp</artifactId>
---
>       <artifactId>kubernetes-httpclient-jdk</artifactId>
693,698d692
<     </dependency>
<     <dependency>
<       <groupId>com.squareup.okhttp3</groupId>
<       <artifactId>okhttp</artifactId>
<       <version>4.12.0</version>
<       <scope>compile</scope>

[k-wall]

Thanks @SamBarker. However when running mvn dependency:tree -Dfabric8.httpclient.impl=jdk -P'!depend-on-okhttp4'. I still see okhttp on module test classpaths at versions 3.12.12 and 4.12.0.

[k-wall]

Just testing out an idea locally. I hope to have a PR soon. Edit: I expressed an idea SamBarker#9

[gyfora]

What's the status of this? Could you guys @SamBarker @k-wall sort out the issues that you have found?
Does this mean that the changes we released in 1.10.0 don't really do anything? (we can then consider adding this to a patch release if there ever will be one)

[SamBarker]

What's the status of this? Could you guys @SamBarker @k-wall sort out the issues that you have found?

@k-wall's fix is merged into this PR and OkHTTP is fully excluded and thus I think it's ready to merge.

Does this mean that the changes we released in 1.10.0 don't really do anything? (we can then consider adding this to a patch release if there ever will be one)

Possibly. On main the operator builds without okHttp on the class path (which was the intention) however the webhook still pulls OkHttp in as a transitive dependency. I heed to check what that means for the actual image.

[SamBarker]

@gyfora Should I open a pr to apply this change to the release-1.10 branch? Or is there another process for potential backports?

[gyfora]

@SamBarker if you would like to backport it for a potential bug fix release, then please open a separate PR for release-1.10 please :)