ci: remove sudo from playwright-e2e-tests container action#6143
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the .github/actions/playwright-e2e-tests composite action to run correctly inside the ghcr.io/camunda/team-distribution/playwright-runner:latest container, where the job runs as root but sudo is not installed.
Changes:
- Remove
sudofrom theapt-get/rm/sedcommands in the “Install system dependencies” step. - Disable
build-essential-enabledin thecommon-toolingaction invocation to avoid internalsudo apt install build-essentialcalls.
60e436c to
533b85a
Compare
hisImminence
left a comment
There was a problem hiding this comment.
crev review
Specialists run: correctness, devils-advocate. Devil's-advocate hypotheses: 0 raised, 0 promoted.
|
|
||
| # Install gettext for envsubst command used by render-e2e-env.sh | ||
| # Note: no sudo — containers run as root, and sudo may not be installed. | ||
| if command -v apt-get &> /dev/null; then |
There was a problem hiding this comment.
P2 · Unconditional sudo removal breaks non-root bare-runner callers (via devils-advocate)
The bare apt-get calls (without sudo) will fail with 'permission denied' if this composite action is ever invoked outside a root container — for example, from a GitHub-hosted runner (ubuntu-latest runs as a non-root user) or a self-hosted runner that doesn't use the playwright-runner image. The current callers both specify options: --user root, so this is safe today. But the action itself carries no guard limiting it to root-only environments. A future caller that omits --user root would get a silent permission failure partway through the retry loop. Recommendation: either (a) add a root-check at the top of the step (if [ "$(id -u)" -eq 0 ]; then apt-get ...; else sudo apt-get ...; fi), or (b) document in the action's description field that it requires a root execution context.
apt-get update && apt-get install -y gettext-base
| echo "Switching to alternative Ubuntu mirror..." | ||
| sudo sed -i 's|http://archive.ubuntu.com|http://mirrors.edge.kernel.org|g' /etc/apt/sources.list 2>/dev/null || true | ||
| sudo sed -i 's|http://security.ubuntu.com|http://mirrors.edge.kernel.org|g' /etc/apt/sources.list 2>/dev/null || true | ||
| sed -i 's|http://archive.ubuntu.com|http://mirrors.edge.kernel.org|g' /etc/apt/sources.list 2>/dev/null || true |
There was a problem hiding this comment.
P2 · Mirror-fallback sed targets /etc/apt/sources.list, which is empty on Debian bookworm (node:24-slim base) (via correctness)
The playwright-runner image is built FROM node:24-slim (Debian 12/bookworm). Debian 12 moved to DEB822 format: the active APT source is /etc/apt/sources.list.d/debian.sources, not /etc/apt/sources.list. The file /etc/apt/sources.list is present but empty, so the sed substitution on lines 94–95 silently no-ops (saved by || true) and the mirror switch never takes effect. The retry-on-mirror-failure logic is therefore non-functional on the primary container target. In practice impact is low because the envsubst early-exit guard means this block rarely runs, but the fallback is misleading and will silently fail if ever reached on a Debian 12 host. Recommendation: add a sed targeting /etc/apt/sources.list.d/debian.sources alongside the existing one, or replace both with a write that creates a fresh DEB822 sources entry pointing at the kernel.org mirror.
sed -i 's|http://archive.ubuntu.com|http://mirrors.edge.kernel.org|g' /etc/apt/sources.list 2>/dev/null || true
| @@ -119,7 +120,7 @@ runs: | |||
| uses: camunda/infra-global-github-actions/common-tooling@28e9ac0ffb3c71c7a1aaa989d5abdad0738c0436 # main | |||
| with: | |||
There was a problem hiding this comment.
P2 · build-essential disabled on the fallback bare-runner path with node-enabled: true (via devils-advocate)
The check-tools guard means this common-tooling step only executes when helm and kubectl are absent — i.e., on a bare runner lacking the pre-built playwright-runner image. On that path, node-enabled: true is still set. All current package.json files under charts/*/test/e2e/ use only pure-JS deps (@playwright/test, dotenv, typescript-eslint), so disabling build-essential is safe today. However, if @camunda/e2e-test-suite (currently pulled as latest) ever introduces a native addon, the bare-runner path will fail with a cryptic node-gyp error. Recommendation: pin @camunda/e2e-test-suite to a non-latest version so any future native-dep introduction is a visible diff, or add a brief comment explaining why native compilation is not needed.
build-essential-enabled: "false"
The playwright-runner container runs as root but does not have sudo installed, causing 'sudo: command not found' failures in CI. - Replace sudo with direct commands in apt-get/rm/sed calls - Disable build-essential-enabled in common-tooling (prevents sudo apt install build-essential, which Playwright tests don't need)
533b85a to
c82208f
Compare
Summary
sudofrom all commands in theplaywright-e2e-testsaction'sInstall system dependenciesstep — theplaywright-runnercontainer runs as root, butsudois not installedbuild-essential-enabledin thecommon-toolingstep — prevents the external action from runningsudo apt install build-essential, which also fails in the container and is not needed for Playwright testsRoot Cause
The
playwright-runner:latestcontainer (ghcr.io/camunda/team-distribution/playwright-runner) is launched with--user rootbut does not include thesudopackage. When the action runssudo apt-get update, it fails withsudo: command not found.Two code paths triggered this:
Install system dependenciesstep (lines 74-94) — allsudocalls replaced with direct commandscommon-toolingaction — whenbuild-essential-enabled: "true", it runssudo apt install build-essentialinternally. Changed to"false"since Playwright tests don't compile native npm modulesTesting
test-local-template.yamlworkflow is not affected — it runs ongcp-core-8-releaseself-hosted runners wheresudois availablecheck-toolsguard (lines 110-116) skips thecommon-toolingstep entirely ifhelmandkubectlare pre-installed, but the currentplaywright-runnerimage does not include them