[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run

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

fgerlits commented on code in PR #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322#discussion_r866680908


##########
docker/test/integration/environment.py:
##########
@@ -30,7 +34,7 @@ def before_scenario(context, scenario):
         return
 
     logging.info("Integration test setup at {time:%H:%M:%S.%f}".format(time=datetime.datetime.now()))
-    context.test = MiNiFi_integration_test(context.image_store)
+    context.test = MiNiFi_integration_test(context.test_id, context.image_store, context.directory_bindings, context.kind)

Review Comment:
   It may be nicer to pass the whole `context` object through all the way from here `-> MiNiFi_integration_test -> DockerTestCluster -> SingleNodeDockerCluster` and let classes take what they need from it along the way.
   ```suggestion
       context.test = MiNiFi_integration_test(context)
   ```



##########
docker/test/integration/MiNiFi_integration_test_driver.py:
##########
@@ -37,18 +36,17 @@
 from minifi.core.utils import decode_escaped_str
 
 
-class MiNiFi_integration_test():
-    def __init__(self, image_store):
-        self.test_id = str(uuid.uuid4())
-        self.cluster = DockerTestCluster(image_store)
+class MiNiFi_integration_test:
+    def __init__(self, test_id, image_store, directory_bindings, kind):

Review Comment:
   `kind` is not clear here: someone reading the code could think it means "type".
   
   I think `kind_proxy` would be better.  Even better would be to call this `kubernetes_proxy` and rename `KindProxy` to `KubernetesProxy`, too, as the fact that we use `kind` is an implementation detail.  (Yes, I know I named it `KindProxy` -- it was a mistake.)



##########
docker/test/integration/MiNiFi_integration_test_driver.py:
##########
@@ -37,18 +36,17 @@
 from minifi.core.utils import decode_escaped_str
 
 
-class MiNiFi_integration_test():
-    def __init__(self, image_store):
-        self.test_id = str(uuid.uuid4())
-        self.cluster = DockerTestCluster(image_store)
+class MiNiFi_integration_test:
+    def __init__(self, test_id, image_store, directory_bindings, kind):

Review Comment:
   `kind` is not clear here: someone reading the code could think it means "type".
   
   I think `kind_proxy` would be better.  Even better would be to call this `kubernetes_proxy` and rename `KindProxy` to `KubernetesProxy`, too, as the fact that we use `kind` is an implementation detail.  (Yes, I know I named it `KindProxy` -- it was a mistake.)



##########
docker/test/integration/minifi/core/KindProxy.py:
##########
@@ -93,14 +102,23 @@ def __create_objects_of_type(self, directory, type):
             file_name_in_container = os.path.join('/var/tmp', file_name)
             self.__copy_file_to_container(full_file_name, 'kind-control-plane', file_name_in_container)
 
-            (code, output) = self.docker_client.containers.get('kind-control-plane').exec_run(['kubectl', 'apply', '-f', file_name_in_container])
+            (code, _) = self.docker_client.containers.get('kind-control-plane').exec_run(['kubectl', 'apply', '-f', file_name_in_container])
             if code != 0:
                 raise Exception("Could not create kubernetes object from file '%s'" % full_file_name)
 
             object_name = file_name.replace(f'.{type}.yml', '')
             found_objects.append(object_name)
         return found_objects
 
+    def __delete_objects_of_type(self, type):
+        for full_file_name in glob.iglob(os.path.join(self.resources_directory, f'*.{type}.yml')):
+            file_name = os.path.basename(full_file_name)
+            file_name_in_container = os.path.join('/var/tmp', file_name)
+
+            (code, _) = self.docker_client.containers.get('kind-control-plane').exec_run(['kubectl', 'delete', '-f', file_name_in_container, '--grace-period=0', '--force'])
+            if code != 0:
+                raise Exception("Could not delete kubernetes object from file '%s'" % file_name_in_container)

Review Comment:
   We could delete the file in `__create_objects_of_type()`, after `kubectl apply` finished successfully.  Then this function, and some other code which calls this function, could be removed.



##########
docker/test/integration/environment.py:
##########
@@ -30,7 +34,7 @@ def before_scenario(context, scenario):
         return
 
     logging.info("Integration test setup at {time:%H:%M:%S.%f}".format(time=datetime.datetime.now()))
-    context.test = MiNiFi_integration_test(context.image_store)
+    context.test = MiNiFi_integration_test(context.test_id, context.image_store, context.directory_bindings, context.kind)

Review Comment:
   It may be nicer to pass the whole `context` object through all the way from here `-> MiNiFi_integration_test -> DockerTestCluster -> SingleNodeDockerCluster` and let classes take what they need from it along the way.
   ```suggestion
       context.test = MiNiFi_integration_test(context)
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

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