fix fabric8io#3149 fabric8io#3127 deprecating updateStatus and enhanc…

…ing replace

also consolidating more util calls, making some of the messages more
consistent, and trying the passed in resourceVersion first in a replace

CHANGELOG.md

@@ -9,6 +9,7 @@
 * Fix #3121: ServiceOperationImpl replace will throw a KubernetesClientException rather than a NPE if the item doesn't exist
 
 #### Improvements
+* Fix #3149: replace(item) will consult the item's resourceVersion for the first PUT attempt when not specifically locked on a resourceVersion
 * Fix #3135: added mock crud support for patch status, and will return exceptions for unsupported patch types
 * Fix #3072: various changes to refine how threads are handled by informers.  Note that the SharedInformer.run call is now blocking when starting the informer.
 * Fix #3143: a new SharedInformerEventListener.onException(SharedIndexInformer, Exception) method is available to determine which informer could not start.
@@ -20,6 +21,10 @@
 * Fix #3142: Add DSL support for missing resources in `operator.openshift.io` and `monitoring.coreos.com` apiGroups
 * Add DSL support for missing resources in `template.openshift.io`, `helm.openshift.io`, `network.openshift.io`, `user.openshift.io` apigroups
 
+#### _**Note**_: Breaking changes in the API
+##### DSL Changes:
+- #3127 `StatusUpdatable.updateStatus` deprecated, please use patchStatus, editStatus, or replaceStatus
+
 ### 5.4.0 (2021-05-19)
 
 #### Bugs

doc/CHEATSHEET.md

@@ -1702,9 +1702,19 @@ CronTabList cronTabList = cronTabClient.inNamespace("default").list();
 ```java
 Boolean isDeleted = cronTabClient.inNamespace("default").withName("my-third-cron-object").delete();
 ```
-- Update Status of `CustomResource`:
+- Replace Status of `CustomResource`:
+```java
+cronTabClient.inNamespace("default").replaceStatus(updatedCronTab);
+```
+- Patch Status of `CustomResource`:
+```java
+// does not require a full instance of the updatedCronTab, will produce a json merge patch based upon what is set in updatedCronTab
+cronTabClient.inNamespace("default").pachStatus(updatedCronTab);
+```
+- Edit Status of `CustomResource`:
 ```java
-cronTabClient.inNamespace("default").updateStatus(updatedCronTab);
+// generates a json patch between the passed in cronTab and the updated result.  Typically you will use a builder to construct a copy from the current and make modifications
+cronTabClient.inNamespace("default").editStatus(cronTab->updatedCronTab);
 ``` 
 - Watch `CustomResource`, (*note:* You need to register your `CustomResource` to `KubernetesDeserializer` otherwise you won't be able to use watch):
 ```java

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/StatusUpdatable.java

@@ -23,6 +23,8 @@ public interface StatusUpdatable<T> {
    *
    * @param item kubernetes object
    * @return updated object
+   * @deprecated please use one of patchStatus, editStatus, or replaceStatus
    */
+  @Deprecated
   T updateStatus(T item);
 }

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/HasMetadataOperation.java

@@ -120,7 +120,7 @@ protected T requireFromServer(ObjectMeta metadata) {
         throw (KubernetesClientException)e.getCause();
       }
     }
-    throw new KubernetesClientException("Name not specified. But operation requires name.");
+    throw new KubernetesClientException("name not specified for an operation requiring one.");
   }
 
   @Override
@@ -140,18 +140,18 @@ protected T replace(T item, boolean status) {
     String fixedResourceVersion = getResourceVersion();
     Exception caught = null;
     int maxTries = 10;
+    String existingResourceVersion = KubernetesResourceUtil.getResourceVersion(item);
     for (int i = 0; i < maxTries; i++) {
       try {
         final String resourceVersion;
         if (fixedResourceVersion != null) {
           resourceVersion = fixedResourceVersion;
+        } else if (i == 0 && existingResourceVersion != null) {
+          // if a resourceVersion is already there, try it first
+          resourceVersion = existingResourceVersion;
         } else {
           T got = requireFromServer(item.getMetadata());
-          if (got.getMetadata() != null) {
-            resourceVersion = got.getMetadata().getResourceVersion();
-          } else {
-            resourceVersion = null;
-          }
+          resourceVersion = KubernetesResourceUtil.getResourceVersion(got);
         }
 
         final UnaryOperator<T> visitor = resource -> {

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/OperationSupport.java

@@ -161,7 +161,7 @@ public URL getResourceUrl(String namespace, String name) throws MalformedURLExce
   public URL getResourceUrl(String namespace, String name, boolean status) throws MalformedURLException {
     if (name == null) {
       if (status) {
-        throw new KubernetesClientException("Name not specified. But operation requires name.");
+        throw new KubernetesClientException("name not specified for an operation requiring one.");
       }
       return getNamespacedUrl(namespace);
     }
@@ -209,7 +209,7 @@ protected <T> String checkNamespace(T item) {
       if (!isResourceNamespaced()) {
         return null;
       } else {
-        throw new KubernetesClientException("Namespace not specified. But operation requires namespace.");
+        throw new KubernetesClientException("namespace not specified for an operation requiring one.");
       }
     } else if (Utils.isNullOrEmpty(itemNs)) {
       return operationNs;

kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/CreateOrReplaceResourceTest.java

@@ -22,6 +22,7 @@
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.kubernetes.api.model.PodBuilder;
 import io.fabric8.kubernetes.api.model.PodList;
+import io.fabric8.kubernetes.api.model.StatusBuilder;
 import io.fabric8.kubernetes.client.KubernetesClient;
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.dsl.MixedOperation;
@@ -198,6 +199,44 @@ void testReplaceFromLoad() throws Exception {
     assertEquals("nginx", requestPod.getMetadata().getName());
   }
 
+  @Test
+  void testReplaceWithItemResourceVersion() throws Exception {
+    server.expect().put().withPath("/api/v1/namespaces/test/configmaps/map1")
+      .andReturn(HttpURLConnection.HTTP_OK, new ConfigMapBuilder()
+      .withNewMetadata().withResourceVersion("1001").and().build()).once();
+
+    // when you try to replace with a resource version specified, it will try the existing one first
+    ConfigMap map = client.configMaps().withName("map1").replace(new ConfigMapBuilder()
+      .withNewMetadata().withName("map1").withResourceVersion("1000").and().build());
+    assertNotNull(map);
+    assertEquals("1001", map.getMetadata().getResourceVersion());
+
+    ConfigMap replacedMap = new ObjectMapper().readerFor(ConfigMap.class).readValue(server.getLastRequest().getBody().inputStream());
+    assertEquals("1000", replacedMap.getMetadata().getResourceVersion());
+  }
+
+  @Test
+  void testReplaceWithItemResourceVersionRetry() throws Exception {
+    server.expect().get().withPath("/api/v1/namespaces/test/configmaps/map1").andReturn(HttpURLConnection.HTTP_OK, new ConfigMapBuilder()
+      .withNewMetadata().withResourceVersion("1000").and().build()).always();
+
+    server.expect().put().withPath("/api/v1/namespaces/test/configmaps/map1")
+      .andReturn(HttpURLConnection.HTTP_CONFLICT, new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build()).once();
+
+    server.expect().put().withPath("/api/v1/namespaces/test/configmaps/map1")
+      .andReturn(HttpURLConnection.HTTP_OK, new ConfigMapBuilder()
+      .withNewMetadata().withResourceVersion("1001").and().build()).once();
+
+    // when you try to replace with a resource version specified, it will try the existing one first
+    ConfigMap map = client.configMaps().withName("map1").replace(new ConfigMapBuilder()
+      .withNewMetadata().withName("map1").withResourceVersion("999").and().build());
+    assertNotNull(map);
+    assertEquals("1001", map.getMetadata().getResourceVersion());
+
+    ConfigMap replacedMap = new ObjectMapper().readerFor(ConfigMap.class).readValue(server.getLastRequest().getBody().inputStream());
+    assertEquals("1000", replacedMap.getMetadata().getResourceVersion());
+  }
+
   @Test
   @DisplayName("Should replace an existing ConfigMap without lock")
   void testReplaceWithoutLock() throws Exception {

kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/TypedClusterScopeCustomResourceApiTest.java

@@ -41,12 +41,12 @@
 
 @EnableKubernetesMockClient
 class TypedClusterScopeCustomResourceApiTest {
-  
+
     KubernetesMockServer server;
     KubernetesClient client;
 
     private MixedOperation<Star, KubernetesResourceList<Star>, Resource<Star>> starClient;
-    
+
     private CustomResourceDefinitionContext crdContext;
 
     @BeforeEach
@@ -130,6 +130,7 @@ void testPropagationPolicyDeletion() throws InterruptedException {
     @Test
     void testStatusUpdate() throws InterruptedException {
       Star updatedStar = getStar();
+      updatedStar.getMetadata().setResourceVersion("1");
       StarStatus starStatus = new StarStatus();
       starStatus.setLocation("M");
       updatedStar.setStatus(starStatus);
@@ -139,9 +140,30 @@ void testStatusUpdate() throws InterruptedException {
 
       starClient.inNamespace("test").updateStatus(updatedStar);
       RecordedRequest recordedRequest = server.getLastRequest();
+      assertEquals(1, server.getRequestCount());
+      assertEquals("PUT", recordedRequest.getMethod());
+      assertEquals("{\"apiVersion\":\"example.crd.com/v1alpha1\",\"kind\":\"Star\",\"metadata\":{\"name\":\"sun\",\"resourceVersion\":\"1\"},\"spec\":{\"type\":\"G\",\"location\":\"Galaxy\"},\"status\":{\"location\":\"M\"}}", recordedRequest.getBody().readUtf8());
+    }
+
+    @Test
+    void testStatusReplace() throws InterruptedException {
+      Star updatedStar = getStar();
+      StarStatus starStatus = new StarStatus();
+      starStatus.setLocation("M");
+      updatedStar.setStatus(starStatus);
+
+      // without the resourceVersion set, it must first do a get for the latest version
+      server.expect().get().withPath("/apis/example.crd.com/v1alpha1/stars/sun").andReturn(200, "{\"apiVersion\":\"example.crd.com/v1alpha1\",\"kind\":\"Star\",\"metadata\":{\"name\":\"sun\",\"resourceVersion\":\"1\"},\"spec\":{\"type\":\"G\",\"location\":\"Galaxy\"},\"status\":{\"location\":\"M\"}}").once();
+      server.expect().put().withPath("/apis/example.crd.com/v1alpha1/stars/sun/status").andReturn(200, "{\"apiVersion\":\"example.crd.com/v1alpha1\",\"kind\":\"Star\",\"metadata\":{\"name\":\"sun\",\"resourceVersion\":\"2\"},\"spec\":{\"type\":\"G\",\"location\":\"Galaxy\"},\"status\":{\"location\":\"M\"}}").once();
+      starClient = client.customResources(Star.class);
+
+      Star replaced = starClient.inNamespace("test").replaceStatus(updatedStar);
+      assertEquals("2", replaced.getMetadata().getResourceVersion());
+      RecordedRequest recordedRequest = server.getLastRequest();
+      // get of the latest version, put of status
+      assertEquals(2, server.getRequestCount());
       assertEquals("PUT", recordedRequest.getMethod());
-      assertEquals("{\"apiVersion\":\"example.crd.com/v1alpha1\",\"kind\":\"Star\",\"metadata\":{\"name\":\"sun\"},\"spec\":{\"type\":\"G\",\"location\":\"Galaxy\"},\"status\":{\"location\":\"M\"}}", recordedRequest.getBody().readUtf8());
-      System.out.println(recordedRequest.getBody().readUtf8());
+      assertEquals("{\"apiVersion\":\"example.crd.com/v1alpha1\",\"kind\":\"Star\",\"metadata\":{\"name\":\"sun\",\"resourceVersion\":\"1\"},\"spec\":{\"type\":\"G\",\"location\":\"Galaxy\"},\"status\":{\"location\":\"M\"}}", recordedRequest.getBody().readUtf8());
     }
 
     private Star getStar() {

kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/TypedCustomResourceApiTest.java

@@ -134,7 +134,6 @@ void testStatusUpdate() throws InterruptedException {
     RecordedRequest recordedRequest = server.getLastRequest();
     assertEquals("PUT", recordedRequest.getMethod());
     assertEquals("{\"apiVersion\":\"demo.k8s.io/v1alpha1\",\"kind\":\"PodSet\",\"metadata\":{\"name\":\"example-podset\"},\"spec\":{\"replicas\":5},\"status\":{\"availableReplicas\":4}}", recordedRequest.getBody().readUtf8());
-    System.out.println(recordedRequest.getBody().readUtf8());
   }
 
   private PodSet getPodSet() {