SITES-46557 - Add GitHub Actions CI/release workflows - #1080
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…reactor resolution mvn help:evaluate and mvn install:install-file both ran from the repo root (a 28-module aggregator) without -N. Without it, Maven resolves the entire reactor's effective POM graph before running the actual goal - slow, and prone to failing on unrelated modules, when ~/.m2 starts empty as it does in test-aem's container (unlike CircleCI, where restore_cache keeps it warm). Add -N so these only touch the root project.
…on warning v1 declares a node20 runtime, which GitHub Actions is deprecating. v3 requires Node 24 (matching what the runner already defaults to) with no input/output changes, so it's a drop-in bump across all five usages.
…st-aem
Drop the pull_request trigger, keeping push only (matches CircleCI's
push-based model): push + pull_request together caused two independent
runs per commit on branches with an open PR, keyed by different
github.ref values so the existing concurrency group never deduped them.
GitHub still associates check runs with a PR by commit SHA regardless
of trigger type, so this loses no PR visibility.
Also add ARTIFACTORY_CLOUD_USER/PASS to test-aem's job-level env.
container.credentials/services.credentials only authenticate the
runner's docker pull of the private qp/aem images - they're not
exposed to job steps - so it-tests.js's Maven calls (via
settings.xml's ${env.ARTIFACTORY_CLOUD_USER}/_PASS) were failing to
authenticate against Artifactory.
test-aem's container starts with an empty ~/.m2 on every run - no cache is shared with the build job (different container/runner), unlike CircleCI's restore_cache/save_cache working off the same key. Without it, it/site's standalone build and the install-file/download-maven-plugin calls in it-tests.js re-download their entire dependency tree from scratch every time, including old plugin chains like content-package-maven-plugin's. Cache is keyed on pom.xml contents and shared across all matrix.aem/type combinations, since they all need the same underlying dependencies.
… always() actions/cache@v4's save step only runs if the job succeeds. Since test-aem was still failing for an unrelated reason, the cache never got a chance to persist, even after re-downloading its entire dependency tree - every retry stayed cold. Split into actions/cache/restore@v4 (early) and actions/cache/save@v4 with if: always() (near the end), so a failing run still saves whatever it managed to download for the next attempt.
…tainer The quick-provider RMI server started inside the aem service image exports its remote object stub using its own self-view of its address, which defaults to localhost when unset. That's harmless on CircleCI (shared network namespace - localhost reaches every container in the job), but on GitHub Actions this service is only reachable from the qp container by its service name (aem), never localhost - so qp.sh start was failing with "Connection refused to host: localhost" right after a successful bind to aem:55555. Set JAVA_TOOL_OPTIONS=-Djava.rmi.server.hostname=aem on the aem service; every JVM started in that container picks it up silently, including the quick-provider server's, so it advertises aem instead of localhost.
…container:/services:
QuickProvider's RMI protocol advertises "localhost" as its own callback
address, which only works if the qp and aem containers share one
network namespace (--network host) - exactly like CircleCI's machine
executor. GitHub Actions' container:/services: keys cannot express
this: combining --network host with a services: container crashes the
runner before the job even starts ("Error: Value cannot be null.
(Parameter 'ContainerId')"). This is a GitHub Actions runner
limitation, not something fixable by adjusting JAVA_TOOL_OPTIONS or
similar - confirmed against aem-cif-guides-venia's
.github/ci/run-containerized-test.sh, which hit and solved this exact
problem first.
- Add run-containerized-test.sh: drives both containers directly via
the Docker CLI on the bare runner, dumping the aem container's logs
via a cleanup trap regardless of outcome (real visibility into it,
finally)
- test-aem now runs on plain ubuntu-latest instead of container:/
services:; cache path updated to /root/.m2/repository since the qp
container now runs as --user root (needed to write into the
bind-mounted workspace)
- Remove the now-moot JAVA_TOOL_OPTIONS RMI hostname override and the
manual "dump aem logs" diagnostic step, both superseded by this
- it-tests.js needs no functional changes: AEM_HOST already defaulted
to localhost, which is correct again now that networking matches
CircleCI exactly. Updated its stale header comments accordingly.
…test-aem it-tests.js already reads these from process.env (matching CircleCI's script), and run-containerized-test.sh already forwards them into the container - but ci.yml never sourced them from secrets, so the chain was incomplete. Optional: it-tests.js already handles them being unset, so this only takes effect once matching repo secrets exist with real values.
maven-failsafe-plugin writes failsafe-summary.xml into
target/failsafe-reports/ alongside the real per-test TEST-*.xml
reports - a separate aggregate-counts file with no <testsuite>
structure. dorny/test-reporter's JUnit parser crashed trying to read
it as a per-test report ("Cannot read properties of undefined (reading
'$')") since the glob matched it too. Narrow the glob to TEST-*.xml.
… container
Selenium specs were failing at startup with "SevereServiceError:
Cannot find module 'chromedriver'". wdio.conf.local.js ("DO NOT
MODIFY") resolves chromedriver via `command -v chromedriver` - a
system binary lookup, not an npm module - and wdio-chromedriver-
service's own package.json doesn't depend on the chromedriver npm
package either. The qp image, unlike CircleCI's browser-tools/install-
browser-tools orb for this same job, doesn't ship Chrome/chromedriver
at all. Since no specs ever ran, the junit reporter never wrote any
report file either, which is why "Publish test results" separately
failed with "No test report files were found" - same root cause, not
a new issue.
Ported from aem-cif-guides-venia's .github/ci/install-chrome.sh, which
hit and solved this exact problem first (including a related root-
sandbox issue: our qp container also runs as --user root, and Chrome's
sandbox refuses to run as root).
- Add install-chrome.sh: installs a matched Chrome + chromedriver pair
as system binaries, then wraps Chrome's launcher and leaf binary with
--no-sandbox
- Add run-integration-test.sh: new entry point run inside the qp
container - runs install-chrome.sh only when TYPE=selenium, then
runs it-tests.js as before
- run-containerized-test.sh now invokes run-integration-test.sh instead
of calling node it-tests.js directly
Same Node 20 deprecation fix as the earlier dorny/test-reporter bump. Both v5/v6 bump the same underlying @actions/artifact dependency to v4.0.0 (confirmed paired release), with no input/output changes.
…SION
Cache save was failing with "EACCES: permission denied, lstat
'/root/.m2/repository'". The path was never actually shared between
host and container: /root/.m2/repository only existed inside the qp
container's own ephemeral layer (gone the instant --rm removed it), or
was the host's real /root (mode 700), inaccessible to the unprivileged
default runner user actions/cache steps run as. Bind-mount the
runner's own ~/.m2 into the container's /root/.m2 in
run-containerized-test.sh, and cache ~/.m2/repository (the host path)
instead of /root/.m2/repository.
Also add two things surfaced while fixing this:
- actions/cache only ever warns on failure, never fails the step, so
this exact bug was silently hiding as an easy-to-miss orange warning
while the job stayed green. Add a "Verify Maven cache is populated"
step before the save that fails loudly (::error::) if the directory
is missing/empty, and a "Report cache restore outcome" step logging
cache-hit after restore (informational only - a miss is normal on
the first run or after a pom.xml change).
- CACHE_VERSION, mirroring CircleCI's `{{ .Environment.CACHE_VERSION }}`
prefix in maven-repo-*: a repository/organization Variable (not
defined here) that can be bumped later to force a fresh cache
lineage without a code change. Defaults to '1' until that Variable
exists.
Same Node 20 deprecation fix as the other actions bumped earlier. No input/output changes, just the Node 24 runtime bump - confirmed against v5.0.0's release notes.
Cache is confirmed working now (verified hitting on subsequent runs). No longer need the informational cache-hit logging step.
test-aem: remove entirely - matrix/user info was mostly redundant with the job name already shown in the Actions UI, and everything depends on Docker being present anyway (if it weren't, run-containerized-test.sh fails immediately with an equally clear error). build: drop the runner os/arch and git ref/sha echoes (never actually used for debugging) and node/npm versions (frontend-maven-plugin downloads its own pinned Node per module, so the system version isn't actually in play here). Keep java -version/mvn -v, which do confirm matrix.java resolved to the intended JDK.
Same Node 20 deprecation fix as the earlier bumps, extended to the actions we hadn't touched yet: - actions/checkout v4 -> v5 (ci.yml and release.yml) - actions/setup-java v4 -> v5 (ci.yml and release.yml) - actions/setup-node v4 -> v5 (ci.yml) - v5's new auto-caching-on- packageManager-field behavior doesn't affect us, confirmed no package.json in this repo declares that field - codecov/codecov-action v4 -> v6 - v5 stayed on Node 20, v6 is the first Node 24 release; carries a bigger internal rewrite (a "Codecov Wrapper" + a token opt-out change) than the other bumps here, but no breaking changes to the token/flags/fail_ci_if_error inputs we use, and fail_ci_if_error: false everywhere means worst case is a silent non-upload, not a broken build - webfactory/ssh-agent v0.9.0 -> v0.10.0 (release.yml) mxschmitt/action-tmate stays on the floating @V3 tag: v3.24 (already under the v3 line) added Node 24 support, so it's already covered - no v4 tag actually exists despite what a search result claimed.
…onfiguration()
v5.5.0 generates a settings.xml that force-disables Maven's interactive
mode, with no opt-out. ci.js's collectConfiguration() (used by build.js)
and parsePom() (used by deploy-queries.js) both rely on that mode - they
pipe `printf '${project.groupId}|...'` into `mvn help:evaluate` (no
-Dexpression flag) specifically to grab it via the interactive prompt
reading from stdin. With that disabled, Maven never reads the prompt,
produces zero output on both stdout and stderr, and the build fails
immediately on the very first pom.xml processed.
CircleCI never hit this since it uses cimg/openjdk images with their own
Maven install, not actions/setup-java, so this is a GitHub-Actions-only
incompatibility. Reverting is safer than rewriting ci.js's shared
Maven-invocation mechanism just to silence a cosmetic Node 20 warning -
v4 was proven working through this entire migration.
GitHub has no native "Tests" tab. Ports aem-cif-guides-venia's .github/ci/test-summary.py: parses JUnit XML from it/http/target/failsafe-reports and ui.tests/test-module/reports, and writes a CircleCI-style Markdown report to $GITHUB_STEP_SUMMARY - an overview table plus, for each failed test, the stack trace and captured server log in a collapsible block. Explicitly distinguishes "no XML found" (environment/startup failure) from "tests ran and failed", and skips failsafe-summary.xml by checking the XML root tag rather than a filename glob. Needs only contents: read, unlike dorny/test-reporter's checks: write. Complementary to dorny/test-reporter, not a replacement - it still gives the Checks-tab/PR status view this doesn't. Report files are chmod'd first since the qp container writes them as --user root.
Cache is confirmed working reliably now. Removing this in favor of keeping the workflow lean, per user preference.
It wasn't reliably creating check runs (likely a GITHUB_TOKEN permission restriction - e.g. fork PRs force checks: write down to read regardless of this workflow's permissions block). Removed in favor of the Job Summary report (test-summary.py) alone, matching aem-cif-guides-venia's own integration/selenium jobs, which never had a Checks-tab step to begin with. Left in place for build/karma/jest/ jest-extension-product-recs, which don't have an equivalent Job Summary replacement yet.
Neither actually protects against a silent failure: setup-java hard- fails if the requested matrix.java version isn't resolvable rather than silently substituting a different one, and setup-java's own step log already prints the installed JDK version. karma's browser-version echo is informational only, same category as other steps already removed this session. No Debug context steps remain anywhere in this workflow now.
The tmate SSH step gave access to the runner, but run-containerized- test.sh's cleanup trap always removed the aem container immediately on exit - by the time anyone connected, there was nothing left to docker exec into. - run-containerized-test.sh: skip docker rm -f in the cleanup trap when KEEP_AEM_CONTAINER=true (still dumps logs either way); exports the container name via GITHUB_ENV for later steps to reference - ci.yml: wire KEEP_AEM_CONTAINER from the existing debug_enabled workflow_dispatch input; move "Debug via SSH (tmate)" to the very last step, after cache save/artifact upload/coverage upload, so those always complete before this potentially-long, open-ended pause instead of risking never running if the session runs long or the job gets cancelled mid-session; add a final cleanup step that force-removes the container once the debug session ends
…ollapsed - collapsed: auto -> always on all dorny/test-reporter steps: reports stay collapsed regardless of pass/fail, click-to-expand only, instead of auto-expanding on failure - test-aem: remove the custom test-summary.py Job Summary step and re-add dorny/test-reporter (with the same list-tests: failed + collapsed: always config as every other job), for one consistent reporting mechanism across the whole workflow. Whether the check run itself reliably appears here depends on the same GITHUB_TOKEN permission conditions as the other jobs - re-added for consistency regardless. - Delete .github/scripts/test-summary.py, no longer referenced anywhere
…eporter list-tests, list-suites, and list-files are independent controls at each level of the report. Setting only list-tests: failed still listed every fully-passing suite and result file in full, which is the clutter that prompted this. Set all three to failed so a suite/file/ test that fully passed doesn't appear in the report at all - only genuine failures do.
…report dorny/test-reporter's list-tests/list-suites/list-files "failed" settings don't suppress the always-present per-suite/file summary table (a structural limitation of its report format, not a config mistake), and list-tests itself has a long-standing unfixed upstream bug (dorny/test-reporter#142) where it doesn't reliably filter passed tests out at all. mikepenz/action-junit-report's skip_success_summary does what those couldn't: the summary table is skipped entirely when everything passes, and only appears when there's a real failure to show. Already proven for this exact purpose in aem-cif-guides-venia's own ci.yml. - build/karma/jest/jest-extension-product-recs: straight swap, with skip_success_summary: true and fail_on_failure: false - test-aem: split into two matrix.type-gated steps instead of one multi-path glob, since this action's report_paths multi-pattern syntax isn't clearly documented and only one path ever has matching files per matrix.type anyway - Kept the TEST-*.xml glob (not *.xml) for the failsafe path, since this action's tolerance for failsafe-summary.xml is unverified
Changed if: always() to if: failure() on all six mikepenz/action- junit-report steps (build, karma, jest, jest-extension-product-recs, and both test-aem type-gated steps). On a passing run, the step is now skipped entirely - no check run gets created at all - rather than creating one that just shows a minimal passed state via skip_success_summary. Only runs (and only then creates a check run) when there's an actual failure to report. failure() correctly reflects the accumulated job status rather than just the immediately preceding step, so a prior failure (e.g. Integration/UI tests) still triggers these even with an always()-gated step (e.g. Fix report file permissions) running successfully in between.
v5 of upload-artifact and v6 of download-artifact only had "preliminary" Node 24 support - their action.yml still declares runs.using: node20 by default, which is why the runner warning kept naming them. v6/v7 respectively are the first versions that actually declare runs.using: node24. Confirmed no input/output changes in either, just the runtime bump. actions/setup-java@v4 still shows the same warning and stays as-is - that one is intentional (v5.5.0 breaks collectConfiguration(), see its existing comment), not something this commit addresses.
…porting Adds one deliberately failing jest test (react-components) to verify the if: failure() -> mikepenz/action-junit-report -> Job Summary reporting chain actually behaves correctly on a real failure. Verified locally: this is the only failing test out of 90 suites/352 tests. TEMPORARY - to be reverted once verified.
Removes the deliberately failing test added to verify the if: failure() -> mikepenz/action-junit-report -> Job Summary reporting chain. Verified locally: back to 89 suites / 351 tests, all passing.
Rewrite the comments across the .github workflows and scripts to describe their own behavior instead of referencing the CircleCI pipeline they were ported from. No functional changes; .circleci/ is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
alwinjoseph02
changed the title
| } | ||
| // Tag latest commit with component release | ||
| ci.sh(`git tag ${tagName} HEAD`); | ||
| ci.sh(`git push origin ${tagName}`); |
Collaborator
There was a problem hiding this comment.
If this runs twice for the same version (e.g. a release retry), git tag ${tagName} fails because the tag already exists and aborts the step. Consider git tag -f or checking for the tag first so retries are idempotent.
Collaborator
Author
There was a problem hiding this comment.
The same behaviour exists in circleci too. And from my understanding it shouldn't run on the same tag again will create other issues
|
|
||
| # if: failure() (not always()): skip this step entirely on a passing run - only | ||
| # run it (and thus only create a check run) when there's an actual failure. | ||
| - name: Publish test results |
Collaborator
There was a problem hiding this comment.
CircleCI never set an explicit timeout in config.yml — it relied on the built-in
per-step default of 10 minutes of no output (no_output_timeout: 10m), which
automatically killed a hung step (e.g. an AEM startup that never comes up).
but GitHub Actions has no no-output timeout and defaults to a 6-hour job limit,
so that safety net is gone after the migration. A hung test-aem job would sit for
6h before failing.
Suggest adding timeout-minutes to each job (especially the AEM integration/selenium
jobs in ci.yml) to restore the bound CircleCI gave us for free, e.g.:
jobs:
test-aem:
runs-on: ubuntu-latest
timeout-minutes: 30
GitHub Actions has no no-output timeout and defaults to a 6h job limit, so a hung job (stuck mvn/docker pull, wedged selenium session, etc.) would sit for 6h before failing. Add a job-level timeout-minutes to each job to restore the bound CircleCI provided implicitly via its 10m no-output default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ad of 6
`aem` was only declared inside `include:`, not as a top-level matrix key, so
GitHub Actions treated it as an added (non-original) key: each include entry
overwrote the previous one's aem/qp_image/aem_image across every {type}
combination, leaving only the last entry (addon) applied to both type values.
The job silently ran 2 legs instead of 6, with classic and lts never executing
while still reporting green.
Declaring `aem: [classic, lts, addon]` alongside `type` makes it an original
matrix key, so each include entry now only merges into the combination whose
aem already matches - restoring all 6 legs with the correct image pairs,
matching CircleCI's integration-test-{655,660,cloudready-with-addon} and
selenium-chrome-{655,660,cloudready-with-addon} jobs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # it/site/pom.xml
Switch the six Publish test results steps from if: failure() to if: always() so the Checks tab always shows a pass/fail/skip summary, matching CircleCI's Tests insights tab. skip_success_summary keeps a passing run's check lightweight - only the counts, with the per-test table appearing only when there's an actual failure to report.
Replace skip_success_summary with detailed_summary + include_passed on all six Publish test results steps, so the Checks tab lists every test case (name, result, failure message/stack trace) on every run instead of just counts on success.
Add annotate_notice: true to all six Publish test results steps. Without it, only failed tests get an annotation - passed tests never reach the per-test console log line printed during parsing (path:line | test name), so a passing run showed nothing beyond the aggregate count. With it, every test case (pass, fail, skip) gets logged directly in that step's own console output - same job page, no separate tab, and not subject to the Checks API's 50-annotation cap (that only limits what's attached to the check's own Annotations list, not what's printed to the step log).
…st failures" This reverts commit b1ac60f.
…orter Add group_suite: true to all six Publish test results steps. detailed_summary + include_passed already list every test case in the job summary; on a job with hundreds of tests (e.g. 1022 on build jdk 11) that table gets very long to scroll. Grouping by test class/suite instead of one row per test method keeps the same information available at a coarser, shorter grain.
Name the six test-aem legs like the CircleCI jobs they replace (integration-test-655/660/cloudready-with-addon and selenium-chrome-655/660/cloudready-with-addon) via a display-only 'label' on each AEM include entry. The matrix values and AEM env var stay classic/lts/addon so it-tests.js's branching is unaffected.
LSantha
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.