-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIFI-12178 add integration-tests and docker-tests github actions #7858
Conversation
6771895
to
e717b53
Compare
da00ccf
to
ec9ec3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on these additional workflows @ChrisSamo632. Having automated execution of working integration tests and Docker builds will be very useful.
On initial review, I raised several implementation questions. At a high level, it would be very useful to run integration tests locally, so it seems like a more Maven-centric approach to integration test selection would be better. That may present some challenges, but I was wondering whether you considered those options. I also have a few questions on some of the particular build steps and scripting commands.
3ca61a6
to
8c6943a
Compare
@exceptionfactory I've rebased from latest I also noticed I'd missed the |
961fc02
to
cb66a8f
Compare
cb66a8f
to
93aab41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the updates @ChrisSamo632. I noted a few more things on this review, in particular, selecting just Azul Zulu as the JDK should simplify the Docker and Integration Test workflows.
strategy: | ||
fail-fast: false | ||
matrix: | ||
# TODO: macos-latest doesn't come with Docker and Colima seems to have problems with Test Containers - see NIFI-12191 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note above on removing the TODO
given the existence of the Jira issue.
@exceptionfactory RE #7858 (comment) (skipping the The last run of the Workflow in this PR that didn't include the Building without the ZIP wouldn't work - the
The Docker Image builds each create a new Image (TAR file) containing the assembled ZIP, then unzips them before deleting the ZIP file - this uses a lot of disk space |
Thanks for the additional background @ChrisSamo632, that makes sense now, it sounds like the current approach is the best option. |
f23663f
to
08f3105
Compare
@exceptionfactory have rebased from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback and making adjustments @ChrisSamo632, this will be a helpful way to improve maintenance visibility for Docker builds and integration tests. +1 merging
Summary
NIFI-12178 add integration-tests and docker-tests github actions
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
[ ] New dependencies are compatible with the Apache License 2.0 according to the License Policy`[ ] New dependencies are documented in applicableLICENSE
andNOTICE
filesDocumentation
[ ] Documentation formatting appears as expected in rendered files