feat(.github/workflows): add container testing workflow#12352
feat(.github/workflows): add container testing workflow#12352srmanda-cs wants to merge 1 commit intoIQSS:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new GitHub Actions workflow to build the Dataverse container stack and run Maven integration tests against it on develop pushes and PRs.
Changes:
- Introduces
.github/workflows/container_testing_workflow.ymlto build Docker images via Maven, start the stack, wait for readiness, configure Dataverse, and run integration tests. - Adds coverage reporting (JaCoCo + Coveralls) and uploads JaCoCo HTML as an artifact.
- Adds diagnostics/cleanup steps (dump logs on failure, stop stack, prune Docker).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -DreuseForks=true \ | ||
| -DthreadCount=1 \ | ||
| -Dparallel=none \ | ||
| -P all-unit-tests package |
There was a problem hiding this comment.
The Maven invocation mixes lifecycle phases (verify and later package) in the same command. Because verify already runs all prior phases including package, this can cause redundant work (and in some setups can re-run parts of the build). Consider dropping the trailing package and only invoking verify (with the desired profile and system properties).
| -P all-unit-tests package | |
| -P all-unit-tests |
| # --------------------------- | ||
| - name: Upload JaCoCo HTML Report | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
This workflow uses actions/upload-artifact@v4 while the rest of the repo has standardized on newer upload-artifact versions (e.g. .github/workflows/maven_unit_test.yml:65 uses @v7). Consider aligning to the same version to avoid inconsistencies and potential deprecation issues.
| uses: actions/upload-artifact@v4 | |
| uses: actions/upload-artifact@v7 |
| jobs: | ||
| main-integration-tests-workflow: | ||
| runs-on: ubuntu-latest | ||
| if: vars.ENABLE_DOCKER_TESTS == 'true' |
There was a problem hiding this comment.
This job can run on pull_request events from forks; given it builds and runs Docker containers for up to 120 minutes, this can be an unnecessary resource hit and a potential abuse vector. Other container workflows gate execution to the upstream repo (e.g., .github/workflows/container_app_push.yml:36-38 uses if: ${{ github.repository_owner == 'IQSS' }}). Consider adding a similar guard (and/or restricting to pull_request_target with safe checkout patterns if secrets are ever needed).
| if: vars.ENABLE_DOCKER_TESTS == 'true' | |
| if: ${{ vars.ENABLE_DOCKER_TESTS == 'true' && github.repository_owner == 'IQSS' }} |
| paths-ignore: | ||
| - "doc/**" | ||
| - "**/*.md" | ||
| - "**/*.txt" | ||
| - ".github/ISSUE_TEMPLATE/**" | ||
| - ".github/*.md" |
There was a problem hiding this comment.
paths-ignore excludes doc/**, but later this workflow reads and edits doc/sphinx-guides/source/_static/util/createsequence.sql. As a result, PRs that only change that SQL file (or nearby fixtures under doc/) will not trigger this workflow, even though the runtime behavior depends on it. Consider removing doc/** from the ignore list or switching to a paths: allowlist that includes this SQL file.
| # We run curl INSIDE the container so the source IP is 127.0.0.1 | ||
| for key in "${!settings[@]}"; do | ||
| echo "Setting $key..." | ||
| docker exec dev_dataverse curl -s -X PUT -d "${settings[$key]}" "http://localhost:8080/api/admin/settings/$key" |
There was a problem hiding this comment.
The docker exec ... curl calls don't use --fail/--fail-with-body (and don't check HTTP status), so a 4xx/5xx response can still return exit code 0 and silently leave the instance misconfigured, causing harder-to-diagnose test failures later. Consider adding --fail-with-body (or --fail) and logging/validating the response for each setting update.
| docker exec dev_dataverse curl -s -X PUT -d "${settings[$key]}" "http://localhost:8080/api/admin/settings/$key" | |
| response="$(docker exec dev_dataverse curl --silent --show-error --fail-with-body -X PUT -d "${settings[$key]}" "http://localhost:8080/api/admin/settings/$key")" | |
| echo "$response" |
| run: | | ||
| set -euo pipefail | ||
| # Fixes MakeDataCountApiIT | ||
| docker exec dev_dataverse sh -c 'curl https://raw.githubusercontent.com/IQSS/dataverse/develop/src/test/java/edu/harvard/iq/dataverse/makedatacount/sushi_sample_logs.json > /tmp/sushi_sample_logs.json && head /tmp/sushi_sample_logs.json' |
There was a problem hiding this comment.
This step downloads a test fixture from raw.githubusercontent.com without pinning to a commit SHA and without curl --fail, which hurts CI reproducibility and can silently succeed on HTTP errors (e.g., 404 HTML saved as JSON). Prefer using a fixture checked into the repo, or pin to a specific commit + add -fL/--fail-with-body so failures are explicit.
| docker exec dev_dataverse sh -c 'curl https://raw.githubusercontent.com/IQSS/dataverse/develop/src/test/java/edu/harvard/iq/dataverse/makedatacount/sushi_sample_logs.json > /tmp/sushi_sample_logs.json && head /tmp/sushi_sample_logs.json' | |
| docker cp src/test/java/edu/harvard/iq/dataverse/makedatacount/sushi_sample_logs.json dev_dataverse:/tmp/sushi_sample_logs.json | |
| docker exec dev_dataverse sh -c 'head /tmp/sushi_sample_logs.json' |
|
Closed in favor of this one: |
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: