From a8780263236dfa4cdffdb3bc3b25d57db63add06 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 17 Jan 2020 09:55:40 +0100 Subject: [PATCH] Don't fail in `deleteIfManaged` when the namespace/project is not managed (#15688) Don't fail in `deleteIfManaged` when the namespace/project is not managed. Instead, ignore the request silently. That seems to be the original intention based on the usage of the method everywhere else but the test suite 8-| Signed-off-by: Lukas Krejci --- .../namespace/KubernetesNamespace.java | 23 ++++++++----------- .../namespace/KubernetesNamespaceTest.java | 8 +------ .../openshift/project/OpenShiftProject.java | 20 ++++++++-------- .../project/OpenShiftProjectTest.java | 8 +------ 4 files changed, 21 insertions(+), 38 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java index 53ad55a88a7..47b67f7055a 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespace.java @@ -141,23 +141,20 @@ void prepare(boolean markManaged, boolean canCreate) throws InfrastructureExcept /** * Deletes the namespace. Deleting a non-existent namespace is not an error as is not an attempt - * to delete a namespace that is already being deleted. + * to delete a namespace that is already being deleted. If the namespace is not marked as managed, + * it is silently not deleted. * - * @throws InfrastructureException if the namespace is not marked managed or when any unexpected - * exception occurs during namespace deletion + * @throws InfrastructureException if any unexpected exception occurs during namespace deletion */ void deleteIfManaged() throws InfrastructureException { KubernetesClient client = clientFactory.create(workspaceId); - if (!isManagedInternal(client)) { - throw new InfrastructureException( - format( - "Can't delete namespace '%s' that contains" - + " runtime of workspace '%s' because it doesn't have the '" - + MANAGED_NAMESPACE_LABEL - + "' label equal to 'true'.", - name, - workspaceId)); + if (!isNamespaceManaged(client)) { + LOG.info( + "Namespace {} for workspace {} is not marked as managed. Ignoring the delete request.", + name, + workspaceId); + return; } try { @@ -175,7 +172,7 @@ void deleteIfManaged() throws InfrastructureException { } } - private boolean isManagedInternal(KubernetesClient client) throws InfrastructureException { + private boolean isNamespaceManaged(KubernetesClient client) throws InfrastructureException { try { Namespace namespace = client.namespaces().withName(name).get(); return namespace.getMetadata().getLabels() != null diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java index ee2d113e56b..561da103a4e 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceTest.java @@ -23,7 +23,6 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; -import static org.testng.Assert.fail; import io.fabric8.kubernetes.api.model.DoneableNamespace; import io.fabric8.kubernetes.api.model.DoneableServiceAccount; @@ -304,12 +303,7 @@ public void testDoesntDeleteExistingNonManagedNamespace() throws Exception { Resource resource = prepareNamespaceResource(NAMESPACE); // when - try { - namespace.deleteIfManaged(); - fail("Deleting a non-managed namespace shouldn't have succeeded."); - } catch (InfrastructureException e) { - // good - } + namespace.deleteIfManaged(); // then verify(resource, never()).delete(); diff --git a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java index f77b2e78065..426ecc72a8c 100644 --- a/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java +++ b/infrastructures/openshift/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProject.java @@ -123,7 +123,8 @@ void prepare(boolean markManaged, boolean canCreate) throws InfrastructureExcept /** * Deletes the project. Deleting a non-existent projects is not an error as is not an attempt to - * delete a project that is already being deleted. + * delete a project that is already being deleted. If the project is not marked as managed, it is + * silently not deleted. * * @throws InfrastructureException if any unexpected exception occurs during project deletion */ @@ -133,15 +134,12 @@ void deleteIfManaged() throws InfrastructureException { OpenShiftClient osClient = clientFactory.createOC(workspaceId); - if (!isManagedInternal(osClient)) { - throw new InfrastructureException( - format( - "Can't delete project '%s' that contains" - + " runtime of workspace '%s' because it doesn't have the '" - + MANAGED_NAMESPACE_LABEL - + "' label equal to 'true'.", - projectName, - workspaceId)); + if (!isProjectManaged(osClient)) { + LOG.debug( + "Project {} for workspace {} is not marked as managed. Ignoring the delete request.", + projectName, + workspaceId); + return; } delete(projectName, osClient); @@ -226,7 +224,7 @@ private Project get(String projectName, OpenShiftClient client) throws Infrastru } } - private boolean isManagedInternal(OpenShiftClient client) throws InfrastructureException { + private boolean isProjectManaged(OpenShiftClient client) throws InfrastructureException { try { Project namespace = client.projects().withName(getName()).get(); return namespace.getMetadata().getLabels() != null diff --git a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java index 9087fc36a7d..0bab093da1f 100644 --- a/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java +++ b/infrastructures/openshift/src/test/java/org/eclipse/che/workspace/infrastructure/openshift/project/OpenShiftProjectTest.java @@ -21,7 +21,6 @@ import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotNull; -import static org.testng.Assert.fail; import io.fabric8.kubernetes.api.model.DoneableServiceAccount; import io.fabric8.kubernetes.api.model.ObjectMeta; @@ -247,12 +246,7 @@ public void testDoesntDeleteExistingNonManagedNamespace() throws Exception { Resource resource = prepareProjectResource(PROJECT_NAME); // when - try { - project.deleteIfManaged(); - fail("Deleting a non-managed project shouldn't have succeeded."); - } catch (InfrastructureException e) { - // good - } + project.deleteIfManaged(); // then verify(resource, never()).delete();