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/07/13 12:50:52 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
martinzink opened a new pull request, #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369
removed options duplication in cmake
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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] 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.
- [X] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [X] Is your initial contribution a single, squashed commit?
### For code changes:
- [X] 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)?
- [X] If applicable, have you updated the LICENSE file?
- [X] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [X] 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] szaszm commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r921862093
##########
docker/DockerVerify.sh:
##########
@@ -67,11 +67,11 @@ TEST_DIRECTORY="${docker_dir}/test/integration"
export TEST_DIRECTORY
# Add --no-logcapture to see logs interleaved with the test output
-BEHAVE_OPTS=(-f pretty --logging-level INFO --logging-clear-handlers --tags ~@no-ci)
Review Comment:
My point is that there would be no harm in keeping the tag, even if unused. The added complexity is minimal, and there may be cases in the future where we need to disable individual (e.g. long running) tests, not the whole extension. It can also be readded, so it's fine if you prefer to still remove it.
--
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 closed pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
lordgamez closed pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r921053846
##########
docker/DockerVerify.sh:
##########
@@ -67,11 +67,11 @@ TEST_DIRECTORY="${docker_dir}/test/integration"
export TEST_DIRECTORY
# Add --no-logcapture to see logs interleaved with the test output
-BEHAVE_OPTS=(-f pretty --logging-level INFO --logging-clear-handlers --tags ~@no-ci)
Review Comment:
I moved the filtering to https://github.com/apache/nifi-minifi-cpp/pull/1369/files#diff-aebf957cd74e48986733a1597cb988264e869aa6709d4c6990e7f3e564e4e384R75
--
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 #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922262730
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -506,7 +507,7 @@ TEST_CASE_METHOD(PutSFTPTestsFixture, "PutSFTP set mtime", "[PutSFTP]") {
testFile("nifi_test/tstFile1.ext", "content 1");
using namespace std::chrono; // NOLINT(build/namespaces)
- time_point<system_clock> modification_time = sys_days(January / 24 / 2065) + 5h + 20min;
+ system_clock::time_point modification_time = date::sys_days(date::January / 24 / 2065) + 5h + 20min;
Review Comment:
Thanks for the info!
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922256892
##########
docker/Dockerfile:
##########
@@ -28,47 +28,7 @@ ARG GID=1000
# PDH and WEL extensions and not listed as they are Windows specific
# SYSTEMD extension is turned OFF explicitly as it has no use in an alpine container
Review Comment:
nice catch, removed this in https://github.com/apache/nifi-minifi-cpp/pull/1369/commits/69bbe6d590f5608d809a28fff7aa062bfd80800b
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922243196
##########
docker/DockerVerify.sh:
##########
@@ -67,11 +67,11 @@ TEST_DIRECTORY="${docker_dir}/test/integration"
export TEST_DIRECTORY
# Add --no-logcapture to see logs interleaved with the test output
-BEHAVE_OPTS=(-f pretty --logging-level INFO --logging-clear-handlers --tags ~@no-ci)
Review Comment:
The problem with the tag was that it influenced local runs as well. https://github.com/apache/nifi-minifi-cpp/pull/1349#discussion_r907500456
Due to this reason Id rather readd it later if we need it.
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r923142594
##########
docker/bionic/Dockerfile:
##########
@@ -20,6 +20,8 @@
FROM ubuntu:bionic AS build_deps
LABEL maintainer="Apache NiFi <de...@nifi.apache.org>"
+# MINIFI_OPTIONS will be passed directly to cmake
+# use it to define cmake options (e.g. -DENABLE_AWS=ON -DENABLE_AZURE=ON)
Review Comment:
you are absolutely right :+1:
fixed in https://github.com/apache/nifi-minifi-cpp/pull/1369/commits/6851250bc6ce26b4c7a0c5824e66cf8d7d16f923
--
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] adamdebreceni commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r923118902
##########
docker/bionic/Dockerfile:
##########
@@ -20,6 +20,8 @@
FROM ubuntu:bionic AS build_deps
LABEL maintainer="Apache NiFi <de...@nifi.apache.org>"
+# MINIFI_OPTIONS will be passed directly to cmake
+# use it to define cmake options (e.g. -DENABLE_AWS=ON -DENABLE_AZURE=ON)
Review Comment:
I think this comment might be misplaced (also some other Dockerfiles)
--
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 #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r921228631
##########
docker/Dockerfile:
##########
@@ -28,47 +28,7 @@ ARG GID=1000
# PDH and WEL extensions and not listed as they are Windows specific
# SYSTEMD extension is turned OFF explicitly as it has no use in an alpine container
-ARG ENABLE_ALL=OFF
-ARG ENABLE_PYTHON=OFF
-ARG ENABLE_OPS=ON
-ARG ENABLE_JNI=OFF
-ARG ENABLE_OPENCV=OFF
-ARG ENABLE_OPC=OFF
-ARG ENABLE_GPS=OFF
-ARG ENABLE_COAP=OFF
-ARG ENABLE_SQL=OFF
-ARG ENABLE_MQTT=OFF
-ARG ENABLE_PCAP=OFF
-ARG ENABLE_LIBRDKAFKA=OFF
-ARG ENABLE_SENSORS=OFF
-ARG ENABLE_USB_CAMERA=OFF
-ARG ENABLE_TENSORFLOW=OFF
-ARG ENABLE_AWS=OFF
-ARG ENABLE_BUSTACHE=OFF
-ARG ENABLE_SFTP=OFF
-ARG ENABLE_OPENWSMAN=OFF
-ARG ENABLE_AZURE=OFF
-ARG ENABLE_ENCRYPT_CONFIG=ON
-ARG ENABLE_NANOFI=OFF
-ARG ENABLE_SPLUNK=OFF
-ARG ENABLE_GCP=OFF
-ARG ENABLE_ELASTICSEARCH=OFF
-ARG ENABLE_TEST_PROCESSORS=OFF
-ARG DISABLE_CURL=OFF
-ARG DISABLE_JEMALLOC=ON
-ARG DISABLE_CIVET=OFF
-ARG DISABLE_EXPRESSION_LANGUAGE=OFF
-ARG DISABLE_ROCKSDB=OFF
-ARG DISABLE_LIBARCHIVE=OFF
-ARG DISABLE_LZMA=OFF
-ARG DISABLE_BZIP2=OFF
-ARG ENABLE_SCRIPTING=OFF
-ARG DISABLE_PYTHON_SCRIPTING=
-ARG ENABLE_LUA_SCRIPTING=
-ARG ENABLE_KUBERNETES=OFF
-ARG ENABLE_PROCFS=OFF
-ARG ENABLE_PROMETHEUS=OFF
-ARG DISABLE_CONTROLLER=OFF
+ARG MINIFI_OPTIONS=""
Review Comment:
Maybe we could add a comment what this argument should look like if someone would want to use this dockerfile individually without using the shell script or a make target.
##########
docker/Dockerfile:
##########
@@ -28,47 +28,7 @@ ARG GID=1000
# PDH and WEL extensions and not listed as they are Windows specific
# SYSTEMD extension is turned OFF explicitly as it has no use in an alpine container
Review Comment:
These comments can be removed
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -506,7 +507,7 @@ TEST_CASE_METHOD(PutSFTPTestsFixture, "PutSFTP set mtime", "[PutSFTP]") {
testFile("nifi_test/tstFile1.ext", "content 1");
using namespace std::chrono; // NOLINT(build/namespaces)
- time_point<system_clock> modification_time = sys_days(January / 24 / 2065) + 5h + 20min;
+ system_clock::time_point modification_time = date::sys_days(date::January / 24 / 2065) + 5h + 20min;
Review Comment:
How is this change tied to this PR?
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922238458
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -506,7 +507,7 @@ TEST_CASE_METHOD(PutSFTPTestsFixture, "PutSFTP set mtime", "[PutSFTP]") {
testFile("nifi_test/tstFile1.ext", "content 1");
using namespace std::chrono; // NOLINT(build/namespaces)
- time_point<system_clock> modification_time = sys_days(January / 24 / 2065) + 5h + 20min;
+ system_clock::time_point modification_time = date::sys_days(date::January / 24 / 2065) + 5h + 20min;
Review Comment:
I found this during development, because it didnt compile on centos (usually this is not a problem because -DSKIP_TESTS is hardcoded to be ON with the centos target)
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922238458
##########
extensions/sftp/tests/PutSFTPTests.cpp:
##########
@@ -506,7 +507,7 @@ TEST_CASE_METHOD(PutSFTPTestsFixture, "PutSFTP set mtime", "[PutSFTP]") {
testFile("nifi_test/tstFile1.ext", "content 1");
using namespace std::chrono; // NOLINT(build/namespaces)
- time_point<system_clock> modification_time = sys_days(January / 24 / 2065) + 5h + 20min;
+ system_clock::time_point modification_time = date::sys_days(date::January / 24 / 2065) + 5h + 20min;
Review Comment:
I found this during development, because it didnt compile on centos (usually this is not a problem because -DSKIP_TESTS is hardcoded to be ON)
--
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 commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r920274736
##########
docker/DockerVerify.sh:
##########
@@ -67,11 +67,11 @@ TEST_DIRECTORY="${docker_dir}/test/integration"
export TEST_DIRECTORY
# Add --no-logcapture to see logs interleaved with the test output
-BEHAVE_OPTS=(-f pretty --logging-level INFO --logging-clear-handlers --tags ~@no-ci)
Review Comment:
Why did you remove the `@no-ci` filtering? I believe there is still value in excluding certain tests from the CI.
--
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] martinzink commented on a diff in pull request #1369: MINIFICPP-1874 docker-verify should only run tests that are enabled
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1369:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1369#discussion_r922256688
##########
docker/Dockerfile:
##########
@@ -28,47 +28,7 @@ ARG GID=1000
# PDH and WEL extensions and not listed as they are Windows specific
# SYSTEMD extension is turned OFF explicitly as it has no use in an alpine container
-ARG ENABLE_ALL=OFF
-ARG ENABLE_PYTHON=OFF
-ARG ENABLE_OPS=ON
-ARG ENABLE_JNI=OFF
-ARG ENABLE_OPENCV=OFF
-ARG ENABLE_OPC=OFF
-ARG ENABLE_GPS=OFF
-ARG ENABLE_COAP=OFF
-ARG ENABLE_SQL=OFF
-ARG ENABLE_MQTT=OFF
-ARG ENABLE_PCAP=OFF
-ARG ENABLE_LIBRDKAFKA=OFF
-ARG ENABLE_SENSORS=OFF
-ARG ENABLE_USB_CAMERA=OFF
-ARG ENABLE_TENSORFLOW=OFF
-ARG ENABLE_AWS=OFF
-ARG ENABLE_BUSTACHE=OFF
-ARG ENABLE_SFTP=OFF
-ARG ENABLE_OPENWSMAN=OFF
-ARG ENABLE_AZURE=OFF
-ARG ENABLE_ENCRYPT_CONFIG=ON
-ARG ENABLE_NANOFI=OFF
-ARG ENABLE_SPLUNK=OFF
-ARG ENABLE_GCP=OFF
-ARG ENABLE_ELASTICSEARCH=OFF
-ARG ENABLE_TEST_PROCESSORS=OFF
-ARG DISABLE_CURL=OFF
-ARG DISABLE_JEMALLOC=ON
-ARG DISABLE_CIVET=OFF
-ARG DISABLE_EXPRESSION_LANGUAGE=OFF
-ARG DISABLE_ROCKSDB=OFF
-ARG DISABLE_LIBARCHIVE=OFF
-ARG DISABLE_LZMA=OFF
-ARG DISABLE_BZIP2=OFF
-ARG ENABLE_SCRIPTING=OFF
-ARG DISABLE_PYTHON_SCRIPTING=
-ARG ENABLE_LUA_SCRIPTING=
-ARG ENABLE_KUBERNETES=OFF
-ARG ENABLE_PROCFS=OFF
-ARG ENABLE_PROMETHEUS=OFF
-ARG DISABLE_CONTROLLER=OFF
+ARG MINIFI_OPTIONS=""
Review Comment:
good idea, I've added comments in https://github.com/apache/nifi-minifi-cpp/pull/1369/commits/69bbe6d590f5608d809a28fff7aa062bfd80800b
--
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