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 2020/08/24 13:56:58 UTC
[GitHub] [nifi-minifi-cpp] lordgamez opened a new pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
lordgamez opened a new pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878
These changes consist of 3 main parts:
- Fix existing integration tests
- Add PublishKafka SSL tests
- Integrate the docker tests to CI
It's advised to review the PR on a per-commit basis.
Commit 7a5e798 is based on https://github.com/apache/nifi-minifi-cpp/pull/697
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 travis-ci 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#discussion_r479240154
##########
File path: docker/DockerVerify.sh
##########
@@ -17,6 +17,12 @@
set -e
+if [[ $# -lt 1 ]]; then
+ echo "Usage:"
+ echo " ./DockerVerify.sh <MINIFI_VERSION>"
Review comment:
👍
##########
File path: docker/test/integration/minifi/__init__.py
##########
@@ -413,7 +426,7 @@ def __init__(self,
self.controller_services = controller_services
self.schedule = {
- 'scheduling strategy': 'EVENT_DRIVEN',
+ 'scheduling strategy': 'TIMER_DRIVEN',
Review comment:
👍 by default, but longer term it would be nice to make it configurable. Sources should definitely be timer driven, in case of consumers (like publishkafka), it's not that straight forward.
##########
File path: docker/test/integration/minifi/test/__init__.py
##########
@@ -166,20 +184,33 @@ def log_nifi_output(self):
stats = container.stats(stream=False)
logging.info('Container stats:\n%s', stats)
- def check_output(self, timeout=5, **kwargs):
Review comment:
I think having kwargs here is useful, as kwargs can contain subdir as a key.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#discussion_r484555055
##########
File path: docker/test/integration/minifi/__init__.py
##########
@@ -413,7 +426,7 @@ def __init__(self,
self.controller_services = controller_services
self.schedule = {
- 'scheduling strategy': 'EVENT_DRIVEN',
+ 'scheduling strategy': 'TIMER_DRIVEN',
Review comment:
A follow-up is perfectly fine.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#issuecomment-688700266
> Looks good, please resolve the conflict that prevents merge at the moment!
>
> After that I'm happy to merge.
Done
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#issuecomment-688748046
After rebase some tests are failing, I will check those.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#discussion_r480091219
##########
File path: docker/test/integration/minifi/__init__.py
##########
@@ -413,7 +426,7 @@ def __init__(self,
self.controller_services = controller_services
self.schedule = {
- 'scheduling strategy': 'EVENT_DRIVEN',
+ 'scheduling strategy': 'TIMER_DRIVEN',
Review comment:
Shall I try to make it configurable in this PR or have a different jira ticket for this issue for the future?
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
arpadboda closed pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
arpadboda commented on a change in pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#discussion_r484555099
##########
File path: docker/test/integration/minifi/test/__init__.py
##########
@@ -166,20 +184,33 @@ def log_nifi_output(self):
stats = container.stats(stream=False)
logging.info('Container stats:\n%s', stats)
- def check_output(self, timeout=5, **kwargs):
Review comment:
Okay, works for me.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #878: MINIFICPP-1104 Add SSL docker tests for PublishKafka and automate them in CI
Posted by GitBox <gi...@apache.org>.
lordgamez commented on a change in pull request #878:
URL: https://github.com/apache/nifi-minifi-cpp/pull/878#discussion_r479631863
##########
File path: docker/test/integration/minifi/test/__init__.py
##########
@@ -166,20 +184,33 @@ def log_nifi_output(self):
stats = container.stats(stream=False)
logging.info('Container stats:\n%s', stats)
- def check_output(self, timeout=5, **kwargs):
Review comment:
I removed it as `subdir` was the only kwarg we used and replaced it with the `subdir` parameter with empty default value, which is a more explicit signature to see what args we can use.
` def check_output(self, timeout=5, subdir=''):`
We can still use it as a kwarg when calling the check_output.
` assert cluster.check_output(30, subdir='success')`
----------------------------------------------------------------
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