refactor: share code for bulk deletion

[trmendes]

What does this PR do?

Refactor code to have one common function shared between different types of delete

Screenshot / video of UI

No Screenshot

What issues does this PR fix or reference?

Fixes #5745

How to test this PR?

1. Delete some Pods, Services, Deployments, Ingress
2. Run Unit Tests

[deboer-tim]

Thanks for the PR, @trmendes. The goal of #5745 was to reduce common code, but by putting the function calls to delete in one place, the typing means that there's almost as much code as before in Util with the switch and const i = item as pattern.

I was thinking the delete utility would be untyped and allow a delete function to be passed in, something like deleteSelection(items: <Type>[], (item:<Type>) => deleteFunction). Then the logic would be much simpler and each list would pass in its own delete function like await deleteSelection(deployments, (d) => { window.kubernetesDeleteDeployment(deployment.name)})

I'm not sure what to do about containers, but maybe multiple delete functions could optionally be passed in so that it can delete in two passes.

[trmendes]

Thank you so much for your suggestion @deboer-tim

I made some changes as a second draft. The common code is all moved inside the common function inside utils, and the component itself decides what is the correct way to handle the deletion.

For the container component, as a simple implementation, what I have in mind is to call the deleteSelection function twice, in order. First for podGroups: ContainerGroupInfoUI and right next to selectedContainers: ContainerInfoUI.

But, taking a look at the component container, I notice that ContainerInfoUI has the property state instead of status, so I'm not yet sure what would be a clean way to do it.

[deboer-tim]

Much better 👍🏼, but I think callback should be on individual items so that await Promise.all() can be part of the function.

Yes, it would be fine IMHO to at least defer container to a second PR (so 'Fixes most of' in this PR to avoid closing the issue). More drastic options I had thought of were renaming status or state to the other :-/ , but that's fairly extreme and would be a separate discussion, might be better options.

[deboer-tim]

Getting better 👍🏼, but if callback is on each individual item then iterating over them and try/catch logic can be shared in delete function.

[trmendes]

I see, and I agree. Promise.all And the try / catch are happening everywhere, so it makes sense to move it to the common function. :)

Thanks again @deboer-tim. Now we have one more draft before I develop some test cases.

I left containerList component with its own try/catch logic because it differs from the others. It is not only there to log the error, but also to update the container object.

One question is still remaining is “what to do when the deletion of an object fails?”

Promise.all(
    selectedItems.map(async item => {
      try {
        await deleteCallback(item);
      } catch (e) {
        console.error(`error while removing ${item.name}`, e);
      }
    }),
  );

At the moment, we are only logging an error message but, to the user, there is no visual feedback. The object status/state was first set to DELETING and, in case of failure, it will stay on that same state.

I suggest setting it as 'FAILURE'. I would like to hear what are your thoughts and concerns to set status to 'FAILURE' in such cases?

[deboer-tim]

At the moment, we are only logging an error message but, to the user, there is no visual feedback. The object status/state was first set to DELETING and, in case of failure, it will stay on that same state.

I suggest setting it as 'FAILURE'. I would like to hear what are your thoughts and concerns to set status to 'FAILURE' in such cases?

Unfortunately, I think setting it to FAILURE would just mean that you're stranded because some of the actions you might take to fix it or try again are going to be disabled, and we still wouldn't tell you anything about why it failed or what to try. That is probably also the case with DELETING forever :) , but I don't think changing that improves the situation. For right now, you should be able to go to another screen and come back to clear things.

So first: let's keep this PR to fixing a single issue, and if you feel strongly I would at most revert to the previous status.

Long term we could probably store any error message in the object like ContainerInfoUI does with actionError, and show this in the UI somewhere - but that is a much bigger refactoring / discussion to have separately.