You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2025/10/22 14:05:26 UTC
[PR] MINIFICPP-2624 Move OPC UA tests to modular docker tests [nifi-minifi-cpp]
lordgamez opened a new pull request, #2052:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2052
- Allow using multiple minifi containers, and use dictionaries for naming and storing containers instead of a list
- Allow using host file resources in test minifi containers
--------------------------------
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
Re: [PR] MINIFICPP-2665 Move OPC UA tests to modular docker tests [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #2052: MINIFICPP-2665 Move OPC UA tests to modular docker tests
URL: https://github.com/apache/nifi-minifi-cpp/pull/2052
--
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
Re: [PR] MINIFICPP-2624 Move OPC UA tests to modular docker tests [nifi-minifi-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2052:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2052#discussion_r2469478646
##########
extensions/sql/tests/features/steps/postgress_server_container.py:
##########
@@ -41,4 +41,4 @@ def deploy(self) -> bool:
def check_query_results(self, query, number_of_rows):
(code, output) = self.exec_run(["psql", "-U", "postgres", "-c", query])
- return code == 0 and str(number_of_rows) + " rows" in output
+ return code == 0 and str(number_of_rows) + " row" in output
Review Comment:
The check for row count is incomplete. It will match both '1 row' and '1 rows', but for counts greater than 1, PostgreSQL outputs 'N rows' (plural). The original code checked for 'rows' which would work for all counts >= 0. This change breaks the check for counts != 1. Change to: `str(number_of_rows) + (' row' if number_of_rows == 1 else ' rows')`
```suggestion
return code == 0 and (str(number_of_rows) + (" row" if number_of_rows == 1 else " rows")) in output
```
##########
behave_framework/src/minifi_test_framework/containers/container.py:
##########
@@ -69,8 +72,8 @@ def deploy(self) -> bool:
"mode": directory.mode
}
for host_file in self.host_files:
- self.volumes[host_file.container_path] = {
- "bind": host_file.host_path,
+ self.volumes[host_file.host_path] = {
+ "bind": host_file.container_path,
Review Comment:
The volume binding is inverted. The dictionary key should be `host_file.host_path` (the source on the host), and the 'bind' value should be `host_file.container_path` (the target in the container). However, based on Docker's volume syntax, this appears correct. The issue is that this contradicts the pattern used for directories above (lines 69-73), where `self.volumes[directory.path]` uses the container path as the key. This inconsistency could lead to confusion or bugs if the directory pattern is the intended standard.
##########
behave_framework/src/minifi_test_framework/containers/minifi_container.py:
##########
@@ -24,8 +24,8 @@
class MinifiContainer(Container):
- def __init__(self, image_name: str, scenario_id: str, network: Network):
- super().__init__(image_name, f"minifi-{scenario_id}", network)
+ def __init__(self, image_name: str, container_name: str, scenario_id: str, network: Network):
+ super().__init__(image_name, f"{container_name}-{scenario_id}", network)
Review Comment:
The constructor signature changed from `__init__(self, image_name: str, scenario_id: str, network: Network)` to add a `container_name` parameter. Ensure all call sites have been updated to pass the new parameter, as this is a breaking change to the API.
##########
behave_framework/src/minifi_test_framework/core/hooks.py:
##########
@@ -32,28 +32,50 @@ def get_minifi_container_image():
return "apacheminificpp:behave"
+def inject_scenario_id(context: MinifiTestContext, step):
+ if "${scenario_id}" in step.name:
+ step.name = step.name.replace("${scenario_id}", context.scenario_id)
+ if getattr(step, "table", None):
+ for row in step.table:
+ row.cells = [cell.replace("${scenario_id}", context.scenario_id) if "${scenario_id}" in cell else cell for cell in row.cells]
+
+
Review Comment:
[nitpick] The scenario_id injection modifies step names and table cells in place. Consider documenting this side effect clearly, as mutating test step data during initialization could make debugging more difficult. Also consider whether this should be applied to step.text (multiline strings) for consistency.
```suggestion
"""
Injects the scenario_id into step data in place.
Mutates the step object by replacing '${scenario_id}' with the actual scenario_id in:
- step.name
- step.table cells (if present)
- step.text (multiline string, if present)
This mutation occurs during scenario initialization and may affect debugging or reporting.
"""
if "${scenario_id}" in step.name:
step.name = step.name.replace("${scenario_id}", context.scenario_id)
if getattr(step, "table", None):
for row in step.table:
row.cells = [cell.replace("${scenario_id}", context.scenario_id) if "${scenario_id}" in cell else cell for cell in row.cells]
if hasattr(step, "text") and step.text and "${scenario_id}" in step.text:
step.text = step.text.replace("${scenario_id}", context.scenario_id)
```
--
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