fix #3912: simplifying extension mocking #3913
Conversation
|
I still need to update the extension test poms to no longer reference the specific mock dependency - done now. |
for now everything that a user could be using was deprecated, rather than removed
|
Do we want to go one step further and add this functionality into the existing EnableKubernetesMockClient rather than introducing the new EnableKubernetesMock? |
I'm missing the point of the So in this case (and if my assumptions are correct), we should keep both annotations, but provide only a single extension that covers everything. |
| ? new KubernetesMockServer(new Context(), new MockWebServer(), responses, new KubernetesMixedDispatcher(responses), a.https()) | ||
| : new KubernetesMockServer(a.https()); | ||
| mock.init(); | ||
| client = new DefaultKubernetesClient(mock.getMockConfiguration()); |
There was a problem hiding this comment.
Why is an explicit client instantiation needed?
There was a problem hiding this comment.
Because we need the functionality that is below the KubernetesClient, see the next call. We could adapt(BaseClient.class) instead if you want to avoid what will be a deprecated call for a while.
There was a problem hiding this comment.
Then maybe we should change the return type of KubernetesMockServer#createClient to BaseClient. Then change that in the future if possible.
There was a problem hiding this comment.
Unfortunately createClient effectively became a user facing method and should not expose -client classes.
There was a problem hiding this comment.
This implies that usage of the ExternalResource won't work correctly in adapting scenarios. We need KubernetesMockServerExtension and KubenetesServer to share the same KubernetesMockServer and client creation logic for that.
I've updated the pr with using just a single annotation / extension. The only difference is that by default all Clients seen in the test class will be created the same way as a KubernetesClient. If that seems like an acceptable change, I'd just reuse the existing annotation. The purpose of the extensions class may shift depending on the resolution of #3922 |
|
@manusa the next commit addressed your concerns and puts everything using the same mechanisms. |
| * --- | ||
| * apiVersion: "networking.istio.io/v1beta1" | ||
| * kind: "VirtualService" | ||
| * metadata: | ||
| * annotations: {} | ||
| * finalizers: [] | ||
| * labels: {} | ||
| * name: "details" | ||
| * ownerReferences: [] | ||
| * spec: | ||
| * hosts: | ||
| * - "details" | ||
| * http: | ||
| * - route: | ||
| * - destination: | ||
| * host: "details" | ||
| * subset: "v1" | ||
| */ |
There was a problem hiding this comment.
Indentation was broken here by spotless 😭
Need to wrap with <pre>{@code ... }</pre>
There was a problem hiding this comment.
The config has
<setting id="org.eclipse.jdt.core.formatter.comment.format_block_comments" value="true"/>
On something like this, I'd opt for just removing from the file - we generally don't have comments with the raw yaml anywhere else.
There was a problem hiding this comment.
I understand that <pre> enclosed Javadoc should not be altered.
Anyway, just remove it then. One way or the other, we shouldn't have invalid YAML.
There was a problem hiding this comment.
I've updated this to use formatter:off / on
extensions/istio/tests/src/test/java/io/fabric8/istio/test/v1beta1/VirtualServiceTest.java
Outdated
Show resolved
Hide resolved
extensions/istio/tests/src/test/java/io/fabric8/istio/test/v1beta1/VirtualServiceTest.java
Outdated
Show resolved
Hide resolved
extensions/istio/tests/src/test/java/io/fabric8/istio/test/v1beta1/VirtualServiceTest.java
Outdated
Show resolved
Hide resolved
extensions/istio/tests/src/test/java/io/fabric8/istio/test/v1beta1/VirtualServiceTest.java
Outdated
Show resolved
Hide resolved
extension-api # Conflicts: # extensions/volumesnapshot/tests/src/test/java/io/fabric8/volumesnapshot/test/crud/VolumeSnapshotClassTest.java
|
SonarCloud Quality Gate failed. |
Description
fix for #3912 - a general approach to mock support. For now everything that a user could be using was deprecated, rather
than removed
As mentioned on the other comment, I did not try to reduce the duplication between the extension classes because the other implementation should go away in a subsequent release.
Type of change
test, version modification, documentation, etc.)
Checklist