You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "martinzink (via GitHub)" <gi...@apache.org> on 2023/03/21 10:27:16 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
martinzink opened a new pull request, #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538
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] lordgamez commented on a diff in pull request #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165506164
##########
.github/workflows/ci.yml:
##########
Review Comment:
@martinzink all right, I'm okay with that
@szaszm unfortunately that will not work due to the previously mentioned container naming conflicts
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165217460
##########
.github/workflows/ci.yml:
##########
Review Comment:
There is already a ticket for running tests in parallel, but as Martin mentioned it requires larger changes on the test framework: https://issues.apache.org/jira/browse/MINIFICPP-1641
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1151744222
##########
.github/workflows/ci.yml:
##########
Review Comment:
Thats a good idea, however that would require a substantial refactor. Currently the tests are referencing the images by hardcoded names(e.g. minifi-cpp-flow) so running multiple tests at the same time is currently unfeasable. I can create a jira ticket to investigate how much effort that refactoring would need, but it is certainly out of the scope of this PR.
This PR saves cloud resources when we inevitably have to rerun something. Instead of reruning the whole building/running process we can rerun only the affected quadrant of tests.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165478090
##########
.github/workflows/ci.yml:
##########
Review Comment:
In the meantime, maybe we could do something like this:
```
make docker-verify-q1 &
make docker-verify-q2 &
make docker-verify-q3 &
make docker-verify-q4 &
wait %1 %2 %3 %4
```
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165458301
##########
.github/workflows/ci.yml:
##########
Review Comment:
Besides the reruns, since the download/upload artifact overhead is minimal separating them vs keeping them together is roughly the same cloud cpu time, but the separated one finishes considerably faster IRL (~3-4 times faster).
When we manage to run these parallel on the same host we can reconsider merging them, but until then I think its a good idea to shorten the overall CI runtime if there are no major drawbacks.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1143496902
##########
.github/workflows/ci.yml:
##########
@@ -223,13 +227,132 @@ jobs:
if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
mkdir build
cd build
- cmake -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
- -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
- -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache ..
+ cmake ${DOCKER_CMAKE_FLAGS} ..
make docker
+ - name: Save docker image
+ run: cd build && docker save -o minifi_docker.tar apacheminificpp:0.13.0
Review Comment:
It wasnt super simple to go with the suggested route, because this versioning-scheme is used/anticipated in multiple places.
Found a simpler method though: [we can simply extract the version from CMake](https://github.com/apache/nifi-minifi-cpp/pull/1538/commits/6a7cd955d9c0e37815ee27ab2dfcf149a0b932cb).
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1151744222
##########
.github/workflows/ci.yml:
##########
Review Comment:
Thats a good idea, however that would require a substantial refactor. Currently the tests are referencing the images by hardcoded names(e.g. minifi-cpp-flow) so running multiple tests at the same time are currently unfeasable. I can create a jira ticket to investigate how much effort that refactoring would need, but it is certainly out of the scope of this PR.
This PR saves cloud resources when we inevitably have to rerun something. Instead of reruning the whole building/running process we can rerun only the affected quadrant of tests.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165231758
##########
.github/workflows/ci.yml:
##########
Review Comment:
I think it's good to separate the test jobs and the build jobs and it would be cool to have that for the other actions too. Although I'm not convinced that we need to rerun these jobs so often that we need to separate them to 4 quadrants. I think it's more common that some of the tests in ctest runs fail than the ones running in docker. I would rather encourage identifying and fixing the flaky tests cases instead of making it easier to rerun them (which would help people disregard the issues).
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1150109878
##########
.github/workflows/ci.yml:
##########
Review Comment:
I think it would be better to run the 4 parallel jobs on the same VM, to save cloud resources. With this change, we save time by only having to wait 30 minutes instead of 2 hours, but the ASF still pays for 2 hours worth of executor time.
Maybe we could play with buffering the output to avoid the interleaving of tests, or leave them interleaved and tag each line with a thread number.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1143262824
##########
.github/workflows/ci.yml:
##########
@@ -223,13 +227,132 @@ jobs:
if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
mkdir build
cd build
- cmake -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
- -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
- -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache ..
+ cmake ${DOCKER_CMAKE_FLAGS} ..
make docker
+ - name: Save docker image
+ run: cd build && docker save -o minifi_docker.tar apacheminificpp:0.13.0
Review Comment:
Can we call this `apacheminificpp:testing` or similar? It's going to be annoying to always have to update the version number here.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1150109878
##########
.github/workflows/ci.yml:
##########
Review Comment:
I think it would be better to run the 4 parallel jobs on the same VM, to save cloud resources. Maybe we could play with buffering the output to avoid the interleaving of tests, or tag each line with a thread number.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1143315975
##########
.github/workflows/ci.yml:
##########
@@ -223,13 +227,132 @@ jobs:
if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
mkdir build
cd build
- cmake -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
- -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
- -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache ..
+ cmake ${DOCKER_CMAKE_FLAGS} ..
make docker
+ - name: Save docker image
+ run: cd build && docker save -o minifi_docker.tar apacheminificpp:0.13.0
Review Comment:
I think this comes from `make docker`. Maybe we could specify the version as a parameter there, and use `testing` for CI docker image builds.
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165621722
##########
.github/workflows/ci.yml:
##########
@@ -302,13 +306,132 @@ jobs:
if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
mkdir build
cd build
- cmake -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
- -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
- -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache ..
+ cmake ${DOCKER_CMAKE_FLAGS} ..
make docker
+ - name: Save docker image
+ run: cd build && docker save -o minifi_docker.tar apacheminificpp:$(grep CMAKE_PROJECT_VERSION:STATIC CMakeCache.txt | cut -d "=" -f2)
+ - name: Upload artifact
+ uses: actions/upload-artifact@v3
+ with:
+ name: minifi_docker
+ path: build/minifi_docker.tar
+ docker_tests_q1:
+ name: "Docker integration tests 1/4"
+ needs: docker_build
+ runs-on: ubuntu-20.04
+ timeout-minutes: 180
+ steps:
+ - id: checkout
+ uses: actions/checkout@v3
+ - id: run_cmake
+ name: Run CMake
+ run: |
+ mkdir build
+ cd build
+ cmake ${DOCKER_CMAKE_FLAGS} ..
+ - name: Download artifact
+ uses: actions/download-artifact@v3
Review Comment:
yes, these simple download/upload artifacts actions (provided by github) are limited to the current workflow
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1165598645
##########
.github/workflows/ci.yml:
##########
@@ -302,13 +306,132 @@ jobs:
if [ -d ~/.ccache ]; then mv ~/.ccache .; fi
mkdir build
cd build
- cmake -DUSE_SHARED_LIBS= -DSTRICT_GSL_CHECKS=AUDIT -DCI_BUILD=ON -DDISABLE_JEMALLOC=ON -DENABLE_AWS=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_MQTT=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON \
- -DENABLE_SPLUNK=ON -DENABLE_GCP=ON -DENABLE_OPC=ON -DENABLE_PYTHON_SCRIPTING=ON -DENABLE_LUA_SCRIPTING=ON -DENABLE_KUBERNETES=ON -DENABLE_TEST_PROCESSORS=ON -DENABLE_PROMETHEUS=ON \
- -DDOCKER_BUILD_ONLY=ON -DDOCKER_CCACHE_DUMP_LOCATION=$HOME/.ccache ..
+ cmake ${DOCKER_CMAKE_FLAGS} ..
make docker
+ - name: Save docker image
+ run: cd build && docker save -o minifi_docker.tar apacheminificpp:$(grep CMAKE_PROJECT_VERSION:STATIC CMakeCache.txt | cut -d "=" -f2)
+ - name: Upload artifact
+ uses: actions/upload-artifact@v3
+ with:
+ name: minifi_docker
+ path: build/minifi_docker.tar
+ docker_tests_q1:
+ name: "Docker integration tests 1/4"
+ needs: docker_build
+ runs-on: ubuntu-20.04
+ timeout-minutes: 180
+ steps:
+ - id: checkout
+ uses: actions/checkout@v3
+ - id: run_cmake
+ name: Run CMake
+ run: |
+ mkdir build
+ cd build
+ cmake ${DOCKER_CMAKE_FLAGS} ..
+ - name: Download artifact
+ uses: actions/download-artifact@v3
Review Comment:
is it guaranteed that this steps downloads the previous step's artifact? could another (PR) run override the artifact?
--
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 #1538: MINIFICPP-2073 Separate docker build from docker tests in CI
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1538:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1538#discussion_r1150109878
##########
.github/workflows/ci.yml:
##########
Review Comment:
I think it would be better to run the 4 parallel jobs on the same VM, to save cloud resources. Maybe we could play with buffering the output to avoid the interleaving of tests, or leave them interleaved and tag each line with a thread number.
--
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