Skip to content

Commit

Permalink
Don't fail in deleteIfManaged when the namespace/project is not man…
Browse files Browse the repository at this point in the history
…aged (#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 <lkrejci@redhat.com>
  • Loading branch information
metlos authored and vparfonov committed Jan 17, 2020
1 parent 0312752 commit a878026
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit a878026

Please sign in to comment.