You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/05/02 15:29:36 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request, #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
lordgamez opened a new pull request, #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322
https://issues.apache.org/jira/browse/MINIFICPP-1765
----------------------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322#discussion_r866850528
##########
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:
I'm not sure I understand. Te purpose of this function is to delete the kubernetes objects defined in the test cluster, cleaning up after each test scenario, so the next test case can use the empty kind cluster. The file needs to be there for the `kubectle delete` command to know which objects to delete.
##########
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:
I'm not sure I understand. Te purpose of this function is to delete the kubernetes objects defined in the test cluster, cleaning up after each test scenario, so the next test case can use the empty kind cluster. The file needs to be there for the `kubectl delete` command to know which objects to delete.
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by 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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322#discussion_r866904174
##########
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:
Good point! Updated in a70e481e4c2519421743005a5a09e90d0b3506dc
##########
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:
Updated in a70e481e4c2519421743005a5a09e90d0b3506dc
--
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322#discussion_r866877570
##########
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:
Right, I misread this, I thought it was only deleting the file. Never mind.
--
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
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322
--
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1322: MINIFICPP-1765 Instantiate kind cluster only once in a test run
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1322:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1322#discussion_r866850528
##########
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:
I'm not sure I understand. The purpose of this function is to delete the kubernetes objects defined in the test cluster, cleaning up after each test scenario, so the next test case can use the empty kind cluster. The file needs to be there for the `kubectl delete` command to know which objects to delete.
--
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