spring-cloud / spring-cloud-deployer-kubernetes

The Spring Cloud Deployer implementation for Kubernetes
Apache License 2.0
157 stars 98 forks source link

Throw exception if pod or job is not exist #473

Closed umutcann closed 2 years ago

cppwfs commented 2 years ago

Hello @umutcann, Thanks for the contribution! I tested the solution and it appears that the exception never is thrown. This is because podsToDelete is not null and the podsToDelete.list().getItems() returns an empty list. Keep in mind the test I ran had other pods running in the cluster.

onobc commented 2 years ago

Hi @umutcann ,

... This is because podsToDelete is not null and the podsToDelete.list().getItems() returns an empty list.

I ❤️ ObjectUtils.isEmpty as it checks for both null and empty. We could adjust the if to

if (jobsToDelete != null && !ObjectUtils.isEmpty(jobsToDelete.list().getItems()) {

Hi @cppwfs ,

This would probably be a good test case to add. However, I don't think we have any places that directly test the cleanup. The abstract IT cleans up after each test and does exercise the cleanup. Would it be worth adding a test specifically for cleanup verification? I'm not sure if this case warrants it or not.

cppwfs commented 2 years ago

Hey @bono007! I agree that is a great test to add.

onobc commented 2 years ago

Hey @bono007! I agree that is a great test to add.

@cppwfs Sounds good. I will submit a PR and ping back here when I do.

onobc commented 2 years ago

Hi @umutcann

Hi

I changed the code locally to use ObjectUtils and verified the before/after w/ the following test added to org.springframework.cloud.deployer.spi.kubernetes.KubernetesTaskLauncherIntegrationIT

@Test
void cleanupForNonExistentTaskThrowsException() {
  KubernetesTaskLauncher kubernetesTaskLauncher = new KubernetesTaskLauncher(new KubernetesDeployerProperties(),
      new KubernetesTaskLauncherProperties(), kubernetesClient);
  assertThatThrownBy(() -> kubernetesTaskLauncher.cleanup("foo"))
      .isInstanceOf(IllegalStateException.class)
      .hasMessage("Cannot delete Pod for task \"%s\" (reason: pod does not exist)", "foo");
}

Do you mind adding this to your code proposal?

Thanks

onobc commented 2 years ago

Hey @bono007! I agree that is a great test to add.

@cppwfs Sounds good. I will submit a PR and ping back here when I do.

Hi @cppwfs - I added the IT here. Please take a scan when you can. Thx.

onobc commented 2 years ago

Closed via https://github.com/spring-cloud/spring-cloud-deployer-kubernetes/commit/8349b8c503b151884a810349569210c6b3e673d8