[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4461: Fix destroying k8s cluster on shared networks

[GitBox <gi...@apache.org>]

DaanHoogland commented on a change in pull request #4461:
URL: https://github.com/apache/cloudstack/pull/4461#discussion_r521278137



##########
File path: plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterDestroyWorker.java
##########
@@ -239,7 +240,10 @@ public boolean destroy() throws CloudRuntimeException {
                 }
             } else {
                 try {
-                    deleteKubernetesClusterNetworkRules();
+                    NetworkVO kubernetesClusterNetwork = networkDao.findById(kubernetesCluster.getNetworkId());
+                    if (kubernetesClusterNetwork != null && kubernetesClusterNetwork.getGuestType() != Network.GuestType.Shared) {
+                        deleteKubernetesClusterNetworkRules();
+                    }

Review comment:
       I know I'm not a nice guy, but can we please have new code in separate methods:
   ```
   private void checkForRulesToDelete(){
       NetworkVO kubernetesClusterNetwork = networkDao.findById(kubernetesCluster.getNetworkId());
       if (kubernetesClusterNetwork != null && kubernetesClusterNetwork.getGuestType() != Network.GuestType.Shared) {
           deleteKubernetesClusterNetworkRules();
       }
   }
   ```
   and then
   ```suggestion
                       checkForRulesToDelete();
   ```
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org