feat(setup-bazel-cache): skip cache restore if MODULE.bazel.lock has changed - #25
Conversation
…changed This will allow improved independent usage of this action.
331d1fb to
ddab95a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
setup-bazel-cache/action.yml:80
- The new automatic decision is not exercised by the pre-merge tests. On
pull_requestandmerge_group, the ref is notrefs/heads/main, so this step is skipped; the addedworkflow_dispatchjob covers only the missing-checkout failure. A regression in lockfile detection or thetrue/falseoutput would therefore pass CI. Add changed- and unchanged-lockfile cases that assert the composite action'sskip-cache-restoreoutput in a cache-writing context.
- id: module-bazel-lock-changed
if: ${{ inputs.skip-cache-restore == 'auto' && steps.cache-save-enabled.outputs.enabled == 'true' }}
uses: tj-actions/changed-files@9426d40962ed5378910ee2e21d5f8c6fcbf2dd96 # v47.0.6
setup-bazel-cache/action.yml:98
- This validation breaks inputs that the previously delegated boolean parser accepted:
True/TRUEandFalse/FALSEwere valid throughgetBooleanInput, but now fail here. Normalize the value before validating it so existing callers are not broken.
true|false|auto) ;;
… skip-cache-restore
…ty workspaces and update documentation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
setup-bazel-cache/action.yml:127
- This cleanliness check fails open: if
git statuserrors (for example because the index is corrupt), command substitution produces no status text and execution proceeds toactions/checkout, which may clean the workspace. Since this block is explicitly protecting caller files, capture the status command's exit code and refuse checkout when cleanliness cannot be verified.
if [[ -n "$(git -C "$GITHUB_WORKSPACE" status --porcelain=v1 --untracked-files=all --ignored=matching)" ]]; then
setup-bazel-cache/action.yml:115
- This fetch has no refspec, so it follows
origin's configured fetch refspec. A normalactions/checkoutremote is configured for all branch heads, which means deepening a shallow main checkout can unexpectedly fetch every branch. This can make automatic mode very expensive in repositories with many branches; deepen only the current branch ref.
if GIT_TERMINAL_PROMPT=0 git -C "$GITHUB_WORKSPACE" fetch --no-tags --deepen=1 origin >/dev/null 2>&1 && \
git -C "$GITHUB_WORKSPACE" rev-parse --verify HEAD^ >/dev/null 2>&1; then
setup-bazel-cache/action.yml:154
- The central automatic-mode branch is not covered: the new tests only accept either
trueorfalsefrom changed-files and never create known changed/unchanged lock-file histories or assert the resulting restore decision. An implementation that always reportsfalsewould pass. Add controlled commit fixtures that verify a lock-file change resolves skip totrueand an unchanged lock file resolves it tofalse.
LOCK_FILE_CHANGED: ${{ steps.changed-lock-file.outputs.any_modified }}
setup-bazel-cache/README.md:60
- The pinned cache implementation only restores and saves via
@actions/cache; it does not call the cache-deletion API. Claiming this action deletes stale entries and listingactions: writecauses consumers to grant an unnecessary write permission. Remove that permission and direct users to the separateprune-cacheaction when deletion is required.
# This action can delete stale cache entries when it saves a new cache. This is optional, and you can use the prune-cache action instead. Deleting caches requires `actions: write` permission.
actions: write
|
Before I forget this again: With the Since there is already a bigger rework ongoing, I favor to revert this PR largely. Making the default branch configurable is something we may want to keep. |
|
See also this comment eclipse-score/cicd-workflows#189 (comment) |
This will allow improved independent usage of this action.
PR size got a little out of hand here... basically this simply adds
skip-cache-restore: auto. Which in turn triggers a local checkout. Which is tricky given that we want to do that only as required and not touch any local files etc.