ci: cache the lg binary and Playwright browsers; align Go with the deploy - #163
Conversation
nnunley
left a comment
There was a problem hiding this comment.
Can you check if ~/.cache is the correct git actions directory for shared build caches? There's also the comment on using GOPATH rather than ~/go
| id: lgcache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/go/bin/let-go |
There was a problem hiding this comment.
Wouldn't using GOPATH be more idiomatic, rather than rather than ~/go?
There was a problem hiding this comment.
Good question. I checked this against both the tool docs and the actual PR run. On Unix, Go’s default GOPATH is $HOME/go, and actions/cache supports ~ expansion in path. The run resolved this to /home/runner/go/bin/let-go and successfully saved the cache. Using GOPATH directly in a with.path value would not be shell expansion; making it dynamic would require an extra step that exports go env GOPATH as an output. Given these jobs are all ubuntu-latest, I think ~/go/bin/let-go is correct as written.
References: Go GOPATH, actions/cache path handling
| id: lgcache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/go/bin/let-go |
There was a problem hiding this comment.
Same concern here with ~/go vs GOPATH
There was a problem hiding this comment.
Checked here as well: this job is also ubuntu-latest, where the default GOPATH is $HOME/go. actions/cache expands ~, and the current test run proved the same path resolves to /home/runner/go/bin/let-go and saves successfully. I’d keep this consistent with the deploy workflow.
| uses: actions/cache@v4 | ||
| with: | ||
| go-version: '1.24' | ||
| path: ~/go/bin/let-go |
There was a problem hiding this comment.
Same result in the test workflow: the run resolved ~/go/bin/let-go to /home/runner/go/bin/let-go, found the installed binary there, and successfully saved both matrix cache entries. I’d keep this path as written unless we intentionally add a separate go env GOPATH output step.
mparrett
left a comment
There was a problem hiding this comment.
Review follow-up after checking the workflow behavior and logs:
- Playwright path:
~/.cache/ms-playwrightis the documented Linux browser location, and all affected jobs useubuntu-latest. The recent deploy log confirms installation under/home/runner/.cache/ms-playwright. One factual correction: that log shows only the first Playwright install downloaded Chromium; the second install reused it. The PR description should therefore say the download repeats once per fresh runner, not twice within the deploy job. Playwright also notes that restoring a browser cache can take about as long as downloading it, so keeping this cache is a performance tradeoff worth validating rather than a correctness requirement. - Preview cache semantics:
pr-preview.ymlruns onpull_request_target. GitHub gives those runs restore-only access to default-branch caches: they can restore an existing cache but cannot save a miss. I recommend usingactions/cache/restore@v4in that workflow and clarifying that preview runs benefit only after a trusted deploy/manual run has populated the main-branch caches. If previews need to populate caches themselves, that needs a trusted cache-warming path or a larger workflow redesign. - Verification: both PR test jobs are green, both
lgbinary caches were saved successfully, andnpm ci --dry-runpasses intests/e2e.
I’m leaving this as a comment rather than requesting changes so we can decide whether the Playwright cache and preview restore-only cleanup belong in this PR.
References: Playwright browser paths, Playwright CI caching guidance, GitHub pull_request_target cache behavior
|
Follow-up implemented in
Validation: The non-required upstream-latest lane resolved the newly released |
|
Follow-up on the GOPATH threads: rereading them, I may have answered a narrower question than you asked. If the intent was "derive the path from - run: echo "GOPATH=$(go env GOPATH)" >> "$GITHUB_ENV"then |
|
I guess it's fine as it is. I'm just being pedantic right now |
…ploy xsofy has no go.sum, so setup-go's dependency cache never engaged and every CI run compiled let-go from source (~1-3 min in each of tests, deploy, and preview). Cache the built binary keyed on the resolved version instead; the latest lane resolves the moving 'latest' to its concrete version first so it caches too. Playwright's chromium (~150MB, downloaded twice in the deploy: smoke + e2e) gets a cache keyed on the two lockfiles. Also: tests now run Go 1.26 to match deploy-pages.yml (the floor lane was testing on 1.24 while the shipped bundle builds with 1.26), and the e2e step uses npm ci against its committed lockfile. Note: pr-preview.yml runs on pull_request_target, which takes the workflow from main — its changes activate after merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
edf27f8 to
d31e489
Compare
|
Review context for this and the other five ready PRs, with suggested order and merge-order notes: #182 |
Review follow-up: the cache paths restated defaults (`~/go`, `~/.cache/ms-playwright`) that nothing in the workflows declared. Both are correct on today's ubuntu-latest runners, which is why the caches work, but correctness rested on a property the files never asserted. A runner or a job-level GOPATH that disagreed would point the cache somewhere `go install` never writes, and the symptom would be a silent permanent miss, which is the exact cost these caches exist to remove. Ask the tools instead. `go env GOPATH` and an explicit PLAYWRIGHT_BROWSERS_PATH go into GITHUB_ENV once per job; the cache steps consume those. Setting the Playwright variable also means the cache path and what `playwright install` actually writes are one value rather than two strings that happen to match. Same values, same behavior; the assumption is now declared rather than verified after the fact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@nnunley Went with your version in You were right that it was an assumption. Both are now resolved once per job into - name: Resolve cache paths
run: |
echo "GOPATH=$(go env GOPATH)" >> "$GITHUB_ENV"
echo "PLAYWRIGHT_BROWSERS_PATH=$HOME/.cache/ms-playwright" >> "$GITHUB_ENV"Setting Verified on the branch: Good to approve if this settles it. |
|
Follow-through from the CI audit on the let-go side (nooga/let-go#581, #583, #584): the xsofy workflows are cheap per run, but their fixed costs repeat on every run and none of them were cached.
Cache the built
lgbinary. xsofy has nogo.sum, so setup-go's dependency cache never engaged — every run of tests, deploy, and preview compiled let-go from source (~1–3 min each). The binary is now cached keyed on the resolved let-go version; the tests'latestlane resolves the moving tag to its concrete version first (go list -m) so it caches on the same footing. A version bump or new release misses the cache once and rebuilds, which is the correct behavior.Cache Playwright's chromium. Each fresh deploy downloaded Chromium and its supporting browser assets into Playwright's shared Linux cache; the later e2e install already reused that directory within the same job. A cache keyed on the two lockfiles avoids repeating the cross-run download while the pinned versions hold. The e2e step also switches to
npm ciagainst its committed lockfile.Go 1.26 in tests. The floor lane was testing on Go 1.24 while
deploy-pages.ymlbuilds the shipped bundle with 1.26 — the version the tests validate now matches the version that ships.One mechanical note:
pr-preview.ymlruns onpull_request_target, which takes its workflow frommain, so the restore steps activate after this merges rather than on this PR's own preview. GitHub givespull_request_targetruns restore-only access to default-branch caches, so previews benefit after a trusted deploy or manual run seeds the matching cache; a preview cache miss installs normally but is not saved.🤖 Generated with Claude Code