fix fabric8io#3972: taking the changes further to remove overloaded load

all parameter operations are now localized to template logic (overloaded get and load) also fixes kubernetesclientbuilder.withconfig
shawkins · Dec 15, 2022 · 5c876a6 · 5c876a6
1 parent 7dfba49
commit 5c876a6
Show file tree

Hide file tree

Showing 14 changed files with 84 additions and 282 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,8 +11,9 @@
 #### New Features
 
 #### _**Note**_: Breaking changes
-* Fix #3972: deprecated Parameterizable and methods on Serialization accepting parameters - that was only needed as a workaround for non-string parameters.  You should instead include those parameter values in the map passed to processLocally.  
-* Fix #3972: WARNING: future client versions will not implicitly process templates as part of the load operation nor will the static yaml and json ObjectMappers be provided via Serialization.
+* Fix #3972: deprecated Parameterizable and methods on Serialization accepting parameters - that was only needed as a workaround for non-string parameters.  You should instead include those parameter values in the map passed to processLocally.
+* Fix #3972: OpenShiftClient.load will no longer implicitly process templates.  Use OpenShiftClient.templates().load instead.
+* Fix #3972: WARNING: future client versions will not provide the static yaml and json ObjectMappersSerialization.
 
 ### 6.3.1-SNAPSHOT
 

diff --git a/...rnetes-client-api/src/main/java/io/fabric8/kubernetes/client/KubernetesClientBuilder.java b/...rnetes-client-api/src/main/java/io/fabric8/kubernetes/client/KubernetesClientBuilder.java
@@ -98,12 +98,12 @@ public KubernetesClientBuilder withConfig(Config config) {
   }
 
   public KubernetesClientBuilder withConfig(String config) {
-    this.config = Serialization.unmarshal(config);
+    this.config = Serialization.unmarshal(config, Config.class);
     return this;
   }
 
   public KubernetesClientBuilder withConfig(InputStream config) {
-    this.config = Serialization.unmarshal(config);
+    this.config = Serialization.unmarshal(config, Config.class);
     return this;
   }
 

diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java
@@ -402,7 +402,7 @@ public Type getType() {
           return refinedType;
         }
       };
-      CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listTypeReference, getParameters());
+      CompletableFuture<L> futureAnswer = handleResponse(httpClient, requestBuilder, listTypeReference);
       return futureAnswer.thenApply(l -> {
         updateApiVersion(l);
         return l;

diff --git a/...nt/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java b/...nt/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java
@@ -20,7 +20,6 @@
 import io.fabric8.kubernetes.api.builder.Visitor;
 import io.fabric8.kubernetes.api.model.DeletionPropagation;
 import io.fabric8.kubernetes.api.model.HasMetadata;
-import io.fabric8.kubernetes.api.model.KubernetesList;
 import io.fabric8.kubernetes.api.model.KubernetesResourceList;
 import io.fabric8.kubernetes.api.model.StatusDetails;
 import io.fabric8.kubernetes.client.Client;
@@ -85,11 +84,6 @@ public List<HasMetadata> waitUntilReady(final long amount, final TimeUnit timeUn
   List<HasMetadata> getItems() {
     Object item = context.getItem();
 
-    if (item instanceof InputStream) {
-      item = Serialization.unmarshal((InputStream) item, Collections.emptyMap());
-      context = context.withItem(item); // late realization of the inputstream
-    }
-
     return asHasMetadata(item).stream()
         .map(meta -> acceptVisitors(meta,
             Collections.emptyList(), this.context))
@@ -250,19 +244,13 @@ protected Readiness getReadiness() {
 
   protected List<HasMetadata> asHasMetadata(Object item) {
     List<HasMetadata> result = new ArrayList<>();
-    if (item instanceof KubernetesList) {
-      result.addAll(((KubernetesList) item).getItems());
-    } else if (item instanceof KubernetesResourceList) {
+    if (item instanceof KubernetesResourceList) {
       result.addAll(((KubernetesResourceList) item).getItems());
     } else if (item instanceof HasMetadata) {
       result.add((HasMetadata) item);
-    } else if (item instanceof String) {
-      return asHasMetadata(Serialization.unmarshal((String) item));
     } else if (item instanceof Collection) {
       for (Object o : (Collection) item) {
-        if (o instanceof HasMetadata) {
-          result.add((HasMetadata) o);
-        }
+        result.add((HasMetadata) o);
       }
     } else if (item != null) {
       throw new IllegalArgumentException("Could not convert item to a list of HasMetadata");

diff --git a/...etes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupport.java b/...etes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupport.java
@@ -363,7 +363,7 @@ protected <T> T handleUpdate(T updated, Class<T> type, boolean status) throws IO
     HttpRequest.Builder requestBuilder = httpClient.newHttpRequestBuilder()
         .put(JSON, JSON_MAPPER.writeValueAsString(updated))
         .url(getResourceURLForWriteOperation(getResourceUrl(checkNamespace(updated), checkName(updated), status)));
-    return handleResponse(requestBuilder, type, getParameters());
+    return handleResponse(requestBuilder, type);
   }
 
   /**
@@ -479,11 +479,7 @@ protected Status handleDeploymentRollback(String resourceUrl, DeploymentRollback
    */
   protected <T> T handleGet(URL resourceUrl, Class<T> type) throws InterruptedException, IOException {
     HttpRequest.Builder requestBuilder = httpClient.newHttpRequestBuilder().url(resourceUrl);
-    return handleResponse(requestBuilder, type, getParameters());
-  }
-
-  protected Map<String, String> getParameters() {
-    return Collections.emptyMap();
+    return handleResponse(requestBuilder, type);
   }
 
   protected <T extends HasMetadata> T handleApproveOrDeny(T csr, Class<T> type) throws IOException, InterruptedException {
@@ -550,29 +546,13 @@ protected <T> T waitForResult(CompletableFuture<T> future) throws IOException {
    * @throws InterruptedException Interrupted Exception
    * @throws IOException IOException
    */
-  protected <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type) throws InterruptedException, IOException {
-    return handleResponse(requestBuilder, type, getParameters());
-  }
-
-  /**
-   * Send an http request and handle the response, optionally performing placeholder substitution to the response.
-   *
-   * @param requestBuilder request builder
-   * @param type type of object
-   * @param parameters a hashmap containing parameters
-   * @param <T> template argument provided
-   *
-   * @return Returns a de-serialized object as api server response of provided type.
-   * @throws IOException IOException
-   */
-  private <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type, Map<String, String> parameters)
-      throws IOException {
+  protected <T> T handleResponse(HttpRequest.Builder requestBuilder, Class<T> type) throws IOException {
     return waitForResult(handleResponse(httpClient, requestBuilder, new TypeReference<T>() {
       @Override
       public Type getType() {
         return type;
       }
-    }, parameters));
+    }));
   }
 
   /**
@@ -587,8 +567,7 @@ public Type getType() {
    * @return Returns a de-serialized object as api server response of provided type.
    */
   protected <T> CompletableFuture<T> handleResponse(HttpClient client, HttpRequest.Builder requestBuilder,
-      TypeReference<T> type,
-      Map<String, String> parameters) {
+      TypeReference<T> type) {
     VersionUsageUtils.log(this.resourceT, this.apiGroupVersion);
     HttpRequest request = requestBuilder.build();
     CompletableFuture<HttpResponse<byte[]>> futureResponse = new CompletableFuture<>();
@@ -600,7 +579,7 @@ protected <T> CompletableFuture<T> handleResponse(HttpClient client, HttpRequest
       try {
         assertResponseCode(request, response);
         if (type != null && type.getType() != null) {
-          return Serialization.unmarshal(new ByteArrayInputStream(response.body()), type, parameters);
+          return Serialization.unmarshal(new ByteArrayInputStream(response.body()), type);
         } else {
           return null;
         }
@@ -801,9 +780,6 @@ public <R1> R1 restCall(Class<R1> result, String... path) {
         throw e;
       }
       return null;
-    } catch (InterruptedException ie) {
-      Thread.currentThread().interrupt();
-      throw KubernetesClientException.launderThrowable(ie);
     } catch (IOException e) {
       throw KubernetesClientException.launderThrowable(e);
     }

diff --git a/...nt/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/PodOperationsImpl.java b/...nt/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/PodOperationsImpl.java
@@ -263,9 +263,6 @@ private boolean handleEvict(HasMetadata eviction) {
       return false;
     } catch (IOException exception) {
       throw KubernetesClientException.launderThrowable(forOperationType("evict"), exception);
-    } catch (InterruptedException interruptedException) {
-      Thread.currentThread().interrupt();
-      throw KubernetesClientException.launderThrowable(forOperationType("evict"), interruptedException);
     }
   }
 

diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/impl/KubernetesClientImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/impl/KubernetesClientImpl.java
@@ -257,14 +257,6 @@ protected KubernetesClientImpl(Config config, BaseClient client) {
     super(config, client);
   }
 
-  public static KubernetesClientImpl fromConfig(String config) {
-    return new KubernetesClientImpl(Serialization.unmarshal(config, Config.class));
-  }
-
-  public static KubernetesClientImpl fromConfig(InputStream is) {
-    return new KubernetesClientImpl(Serialization.unmarshal(is, Config.class));
-  }
-
   @Override
   public NamespacedKubernetesClient inNamespace(String name) {
     return newInstance(createInNamespaceConfig(name, false));
@@ -307,7 +299,7 @@ public NonNamespaceOperation<ComponentStatus, ComponentStatusList, Resource<Comp
    */
   @Override
   public ParameterNamespaceListVisitFromServerGetDeleteRecreateWaitApplicable<HasMetadata> load(InputStream is) {
-    return resourceListFor(is);
+    return resourceListFor(Serialization.unmarshal(is));
   }
 
   /**
@@ -344,7 +336,7 @@ public NamespaceListVisitFromServerGetDeleteRecreateWaitApplicable<HasMetadata>
    */
   @Override
   public ParameterNamespaceListVisitFromServerGetDeleteRecreateWaitApplicable<HasMetadata> resourceList(String s) {
-    return resourceListFor(s);
+    return resourceListFor(Serialization.unmarshal(s));
   }
 
   @Override

diff --git a/...etes-client/src/test/java/io/fabric8/kubernetes/client/impl/KubernetesClientImplTest.java b/...etes-client/src/test/java/io/fabric8/kubernetes/client/impl/KubernetesClientImplTest.java
@@ -22,6 +22,7 @@
 import io.fabric8.kubernetes.client.Config;
 import io.fabric8.kubernetes.client.ConfigBuilder;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClientBuilder;
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.dsl.ParameterNamespaceListVisitFromServerGetDeleteRecreateWaitApplicable;
 import io.fabric8.kubernetes.client.http.BasicBuilder;
@@ -52,7 +53,6 @@
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @author wangyushuai2@jd.com
@@ -117,21 +117,19 @@ void loadWithWindowsLineSeparatorsString() throws Exception {
   }
 
   @Test
-  void shouldInstantiateClientUsingYaml() {
+  void shouldInstantiateClientUsingYaml() throws Exception {
     File configYml = new File(TEST_CONFIG_YML_FILE);
     try (InputStream is = new FileInputStream(configYml)) {
-      KubernetesClient client = KubernetesClientImpl.fromConfig(is);
+      KubernetesClient client = new KubernetesClientBuilder().withConfig(is).build();
       assertEquals("http://some.url/", client.getMasterUrl().toString());
-    } catch (Exception e) {
-      fail();
     }
   }
 
   @Test
   void shouldInstantiateClientUsingSerializeDeserialize() {
     KubernetesClientImpl original = new KubernetesClientImpl();
     String json = Serialization.asJson(original.getConfiguration());
-    KubernetesClientImpl copy = KubernetesClientImpl.fromConfig(json);
+    KubernetesClient copy = new KubernetesClientBuilder().withConfig(json).build();
 
     assertEquals(original.getConfiguration().getMasterUrl(), copy.getConfiguration().getMasterUrl());
     assertEquals(original.getConfiguration().getOauthToken(), copy.getConfiguration().getOauthToken());

diff --git a/...rnetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/OpenShiftLoadTest.java b/...rnetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/OpenShiftLoadTest.java
@@ -16,31 +16,35 @@
 package io.fabric8.openshift.client.server.mock;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient;
+import io.fabric8.kubernetes.client.server.mock.KubernetesMockServer;
 import io.fabric8.openshift.client.OpenShiftClient;
 import org.junit.jupiter.api.Test;
 
+import java.util.Collections;
 import java.util.List;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
-@EnableOpenShiftMockClient
+@EnableKubernetesMockClient
 class OpenShiftLoadTest {
 
-  OpenShiftMockServer server;
+  KubernetesMockServer server;
   OpenShiftClient client;
 
   @Test
   void testResourceGetFromLoadWhenSingleDocumentsWithStartingDelimiter() {
 
     // when
-    List<HasMetadata> result = client.load(getClass().getResourceAsStream("/test-template.yml")).get();
+    List<HasMetadata> result = client.templates().load(getClass().getResourceAsStream("/test-template.yml"))
+        .processLocally(Collections.emptyMap()).getItems();
 
     // then
     assertNotNull(result);
     assertEquals(5, result.size());
     HasMetadata deploymentResource = result.get(2);
-    assertEquals("image.openshift.io/v1", deploymentResource.getApiVersion());
+    assertEquals("v1", deploymentResource.getApiVersion());
     assertEquals("ImageStream", deploymentResource.getKind());
     assertEquals("eap-app", deploymentResource.getMetadata().getName());
   }
@@ -49,7 +53,8 @@ void testResourceGetFromLoadWhenSingleDocumentsWithStartingDelimiter() {
   void testResourceGetFromLoadWhenSingleDocumentsWithoutDelimiter() {
 
     // when
-    List<HasMetadata> result = client.load(getClass().getResourceAsStream("/template-with-params.yml")).get();
+    List<HasMetadata> result = client.templates().load(getClass().getResourceAsStream("/template-with-params.yml")).get()
+        .getObjects();
 
     // then
     assertNotNull(result);
@@ -59,4 +64,4 @@ void testResourceGetFromLoadWhenSingleDocumentsWithoutDelimiter() {
     assertEquals("Pod", deploymentResource.getKind());
     assertEquals("example-pod", deploymentResource.getMetadata().getName());
   }
-}
+}
diff --git a/...c/main/java/io/fabric8/openshift/client/dsl/internal/build/BuildConfigOperationsImpl.java b/...c/main/java/io/fabric8/openshift/client/dsl/internal/build/BuildConfigOperationsImpl.java
@@ -269,7 +269,7 @@ protected Build submitToApiServer(InputStream inputStream, long contentLength) {
         public Type getType() {
           return Build.class;
         }
-      }, null));
+      }));
     } catch (Throwable e) {
       // TODO: better determine which exception this should occur on
       // otherwise we need to have the httpclient api open up to the notion