Copilot commented on code in PR #2099:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2099#discussion_r2774151550
##########
extensions/grafana-loki/tests/features/steps/grafana_loki_container.py:
##########
@@ -126,8 +126,8 @@ def __init__(self, test_context: MinifiTestContext,
options: GrafanaLokiOptions)
self.files.append(File("/etc/loki/local-config.yaml",
grafana_loki_yml_content.encode(), permissions=0o644))
- def deploy(self):
- super().deploy()
+ def deploy(self, context: MinifiTestContext | None) -> bool:
+ super().deploy(context)
Review Comment:
The wait_for_condition call on lines 132-136 still passes context=None
instead of context. This defeats the purpose of this PR, which is to enable
logging of all container outputs when a container fails to start. The context
parameter should be passed through to wait_for_condition.
##########
extensions/kafka/tests/features/steps/kafka_server_container.py:
##########
@@ -88,8 +88,8 @@ def __init__(self, test_context: MinifiTestContext):
"""
self.files.append(File("/opt/kafka/config/kafka_jaas.conf",
jaas_config_file_content, permissions=0o644))
- def deploy(self):
- super().deploy()
+ def deploy(self, context: MinifiTestContext | None) -> bool:
+ super().deploy(context)
Review Comment:
The wait_for_condition call on lines 94-98 still passes context=None instead
of context. This defeats the purpose of this PR, which is to enable logging of
all container outputs when a container fails to start. The context parameter
should be passed through to wait_for_condition. Note: This issue also exists in
wait_for_kafka_consumer_to_be_registered method on line 126, but that's outside
the scope of the current changes.
##########
extensions/elasticsearch/tests/features/steps/opensearch_container.py:
##########
@@ -43,8 +43,8 @@ def __init__(self, test_context: MinifiTestContext):
features_dir = Path(__file__).resolve().parent.parent
self.host_files.append(HostFile('/usr/share/opensearch/config/opensearch.yml',
os.path.join(features_dir, "resources", "opensearch.yml")))
- def deploy(self):
- return super().deploy('Hot-reloading of audit configuration is
enabled')
+ def deploy(self, context: MinifiTestContext | None) -> bool:
+ return super().deploy(context, 'Hot-reloading of audit configuration
is enabled')
Review Comment:
This method requires 2 positional arguments, whereas overridden
[ElasticBaseContainer.deploy](1) requires 3.
```suggestion
def deploy(self, context: MinifiTestContext | None, message: str =
'Hot-reloading of audit configuration is enabled') -> bool:
return super().deploy(context, message)
```
##########
behave_framework/src/minifi_test_framework/containers/container.py:
##########
@@ -74,7 +79,7 @@ def _configure_volumes_of_container_dirs(self):
self._write_content_to_file(file_path, None, content)
self.volumes[temp_path] = {"bind": directory.path, "mode":
directory.mode}
- def deploy(self) -> bool:
+ def deploy(self, context: "Union[MinifiTestContext, None]") -> bool:
Review Comment:
The type hint uses Union[MinifiTestContext, None] with a forward reference
string, while all the subclass implementations use the modern MinifiTestContext
| None syntax. For consistency, consider using context: "MinifiTestContext |
None" instead of context: "Union[MinifiTestContext, None]". Both are
functionally equivalent, but the | syntax is more concise and matches the style
used throughout the codebase.
```suggestion
def deploy(self, context: "MinifiTestContext | None") -> bool:
```
##########
extensions/elasticsearch/tests/features/steps/elasticsearch_container.py:
##########
@@ -53,8 +53,8 @@ def __init__(self, test_context: MinifiTestContext):
self.environment.append("ELASTIC_PASSWORD=password")
- def deploy(self):
- return super().deploy('"current.health":"GREEN"')
+ def deploy(self, context: MinifiTestContext | None) -> bool:
+ return super().deploy(context, '"current.health":"GREEN"')
Review Comment:
This method requires 2 positional arguments, whereas overridden
[ElasticBaseContainer.deploy](1) requires 3.
```suggestion
def deploy(self, context: MinifiTestContext | None, expected_health: str
= '"current.health":"GREEN"') -> bool:
return super().deploy(context, expected_health)
```
##########
extensions/gcp/tests/features/steps/fake_gcs_server_container.py:
##########
@@ -26,8 +26,8 @@ def __init__(self, test_context: MinifiTestContext):
command=f'-scheme http -host
fake-gcs-server-{test_context.scenario_id}')
self.dirs.append(Directory(path="/data/test-bucket",
files={"test-file": "preloaded data\n"}))
- def deploy(self):
- super().deploy()
+ def deploy(self, context: MinifiTestContext | None) -> bool:
+ super().deploy(context)
Review Comment:
The wait_for_condition call on lines 32-36 still passes context=None instead
of context. This defeats the purpose of this PR, which is to enable logging of
all container outputs when a container fails to start. The context parameter
should be passed through to wait_for_condition.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
