[ENG-603] Fix caching in care backend build actions#3696
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a plugin-hash resolver, switches reusable-test and deploy builds to GHCR-backed Buildx caches, passes the resolved hash into Docker builds, and adds a deploy job that deletes older GHCR build cache versions. ChangesBuild cache and plugin hash plumbing
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…n staleness mitigation
- Replace actions/cache + local buildx cache with type=registry GHCR cache
- Use weekly-rotated cache tags (buildcache-{platform}-{year-Wweek})
- Add prune-cache job to delete tags older than 4 weeks on develop
- Add resolve_plugins.py to compute PLUGIN_RESOLVED_HASH from @Branch refs
for cache busting when plugin upstreams change
- Add PLUGIN_RESOLVED_HASH ARG/ENV to both Dockerfiles
- Remove buildkit-cache-dance and all GHA cache boilerplate from test workflow
- Conditional cache-to in reusable-test.yml (write only on push)
tellmeY18
force-pushed
the
fix/workflow-cache
branch
from
9c55d2e to
9d9a3ea
Compare
Greptile SummaryThis PR replaces the GHA filesystem cache +
Confidence Score: 5/5Safe to merge — the changes are limited to CI/CD infrastructure and Dockerfile ARG ordering; no application code is touched. All three workflow changes are straightforward substitutions (filesystem cache → registry cache). The resolve_plugins.py script handles all edge cases (empty input, JSON parse failure, missing package_name, network errors) with explicit fallbacks. The Dockerfile ARG placement is the standard Docker pattern for cache invalidation. The prune-cache job is guarded to the canonical repo and the ISO-week string comparison is lexicographically correct. No logic paths were left broken or unguarded by this change. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Set default value for PLUGIN_RESOLVED_HA..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| --build-arg PLUGIN_RESOLVED_HASH=${{ steps.plugin-hash.outputs.resolved_hash }} \ | ||
| --build-arg ADDITIONAL_PLUGS=${{ env.ADDITIONAL_PLUGS }} \ |
There was a problem hiding this comment.
ADDITIONAL_PLUGS causes shell word-splitting
${{ env.ADDITIONAL_PLUGS }} is expanded by the Actions expression engine before the shell runs, so a JSON value that contains spaces (e.g. [{"package_name": "...", "version": "@main"}]) gets split into multiple tokens. Docker receives several malformed arguments instead of a single key=value pair and the build fails. The fix is to wrap the argument in double quotes: "--build-arg=ADDITIONAL_PLUGS=${{ env.ADDITIONAL_PLUGS }}".
Note: PLUGIN_RESOLVED_HASH is always a 16-char hex string or no-plugins, so it is safe unquoted — only the JSON-valued arg needs this treatment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/reusable-test.yml (1)
24-29: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: unpinned
docker/login-action@v3.zizmor flags Line 25 as unpinned per a blanket-hash policy. It's consistent with the rest of this workflow's tag-pinned actions, so this is only worth addressing if you intend to enforce SHA pinning repo-wide.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-test.yml around lines 24 - 29, The GitHub Container Registry login step uses an action pinned only by tag, which may violate the repo’s SHA-pin policy. If this workflow should follow the same enforcement as the other tag-pinned actions, update the Docker login step in the reusable test workflow to use a fixed commit SHA for docker/login-action instead of `@v3`, keeping the existing Login to GitHub Container Registry block and its inputs unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/resolve_plugins.py:
- Around line 26-42: The plugin ref resolution in resolve_plugins.py is silently
falling back to the raw branch/ref when git ls-remote fails or returns no
output, which makes values like pkg@main stop changing and hides resolution
problems; update the logic around the ref handling to surface the failure
instead of assigning sha from ref. Also fix the version defaulting in the
package version lookup so a null version is treated like missing input by using
the same default path as the versionless case, and make sure the
ver.startswith("@") branch in the resolver no longer crashes on null values.
In @.github/workflows/reusable-test.yml:
- Around line 42-54: The build step in the reusable workflow is passing
ADDITIONAL_PLUGS directly from the GitHub expression into the shell, which can
strip JSON quotes and split the value before docker buildx receives it. Move
ADDITIONAL_PLUGS into the step env and reference it as a quoted shell variable
in the Build images run block, using the existing docker buildx build invocation
and keeping PLUGIN_RESOLVED_HASH unchanged.
In `@docker/prod.Dockerfile`:
- Around line 37-40: The builder-stage plugin install step in
docker/prod.Dockerfile is not using PLUGIN_RESOLVED_HASH, so changes to the
resolved plugin set do not invalidate the install_plugins.py layer. Update the
RUN step that invokes install_plugins.py to reference PLUGIN_RESOLVED_HASH
alongside the existing ARGs, using the same pattern as the corresponding
dev.Dockerfile cache-busting fix, so the layer is rebuilt whenever the hash
changes.
---
Nitpick comments:
In @.github/workflows/reusable-test.yml:
- Around line 24-29: The GitHub Container Registry login step uses an action
pinned only by tag, which may violate the repo’s SHA-pin policy. If this
workflow should follow the same enforcement as the other tag-pinned actions,
update the Docker login step in the reusable test workflow to use a fixed commit
SHA for docker/login-action instead of `@v3`, keeping the existing Login to GitHub
Container Registry block and its inputs unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8740dfbc-e1d4-4a9b-896b-a767c37f5131
📒 Files selected for processing (5)
.github/scripts/resolve_plugins.py.github/workflows/deploy.yml.github/workflows/reusable-test.ymldocker/dev.Dockerfiledocker/prod.Dockerfile
| if ver.startswith("@"): | ||
| ref = ver[1:] | ||
| git_url = pkg.removeprefix("git+") | ||
| try: | ||
| out = subprocess.run( # noqa: S603 | ||
| ["git", "ls-remote", git_url, ref], # noqa: S607 | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=15, | ||
| ) | ||
| sha = out.stdout.split()[0] if out.stdout else ref | ||
| except Exception: | ||
| sha = ref | ||
| resolved.append(f"{pkg}@{sha}") | ||
| else: | ||
| resolved.append(f"{pkg}{ver}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/scripts/resolve_plugins.py')
print(p.exists(), p)
print(p.read_text())
PYRepository: ohcnetwork/care
Length of output: 1395
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/scripts/resolve_plugins.py')
print(p.exists(), p)
print(p.read_text())
PYRepository: ohcnetwork/care
Length of output: 1395
Avoid falling back to the raw ref here. When git ls-remote fails or returns nothing, sha becomes ref, so @main hashes like pkg@main and stops busting the cache when the branch moves. Surface the failure instead of quietly pretending it resolved.
Also, p.get("version", "@main") does not cover "version": null; that still reaches ver.startswith("@") and crashes. Use p.get("version") or "@main" if null should mean default.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 29-35: Command coming from incoming request
Context: subprocess.run( # noqa: S603
["git", "ls-remote", git_url, ref], # noqa: S607
check=False,
capture_output=True,
text=True,
timeout=15,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.18)
[warning] 38-38: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/resolve_plugins.py around lines 26 - 42, The plugin ref
resolution in resolve_plugins.py is silently falling back to the raw branch/ref
when git ls-remote fails or returns no output, which makes values like pkg@main
stop changing and hides resolution problems; update the logic around the ref
handling to surface the failure instead of assigning sha from ref. Also fix the
version defaulting in the package version lookup so a null version is treated
like missing input by using the same default path as the versionless case, and
make sure the ver.startswith("@") branch in the resolver no longer crashes on
null values.
Source: Linters/SAST tools
| - name: Build images | ||
| run: | | ||
| CACHE_TO="" | ||
| if [[ '${{ github.event_name }}' == 'push' ]]; then | ||
| CACHE_TO="--cache-to=type=registry,ref=ghcr.io/${{ github.repository }}:$CACHE_TAG,mode=max" | ||
| fi | ||
| docker buildx build \ | ||
| --file docker/dev.Dockerfile \ | ||
| --tag care_local \ | ||
| --cache-from=type=local,src=${{ runner.temp }}/.buildx-cache \ | ||
| --cache-to=type=local,dest=${{ runner.temp }}/.buildx-cache-new,mode=max \ | ||
| --build-arg PLUGIN_RESOLVED_HASH=${{ steps.plugin-hash.outputs.resolved_hash }} \ | ||
| --build-arg ADDITIONAL_PLUGS=${{ env.ADDITIONAL_PLUGS }} \ | ||
| --cache-from=type=registry,ref=ghcr.io/${{ github.repository }}:$CACHE_TAG \ | ||
| $CACHE_TO \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow and related references.
git ls-files .github/workflows/reusable-test.yml .github/workflows/deploy.yml
printf '\n--- reusable-test.yml (relevant section) ---\n'
cat -n .github/workflows/reusable-test.yml | sed -n '1,180p'
printf '\n--- search ADDITIONAL_PLUGS / plugin-hash / CACHE_TAG ---\n'
rg -n "ADDITIONAL_PLUGS|plugin-hash|CACHE_TAG|build-args|build-arg" .github/workflows -S
printf '\n--- deploy.yml relevant section ---\n'
cat -n .github/workflows/deploy.yml | sed -n '1,260p'Repository: ohcnetwork/care
Length of output: 15953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where ADDITIONAL_PLUGS is defined and what shape it has.
rg -n "ADDITIONAL_PLUGS" -S .
printf '\n--- files containing plugin hash step ---\n'
rg -n "plugin-hash|resolved_hash" -S .github/workflows .Repository: ohcnetwork/care
Length of output: 1501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe shell behavior with representative JSON-like ADDITIONAL_PLUGS values.
python3 - <<'PY'
import shlex
samples = [
'[]',
'["a"]',
'["a b","c"]',
'["x","y z"]',
'["quote\\"here"]',
]
for s in samples:
print("SAMPLE:", s)
cmd = f'echo --build-arg ADDITIONAL_PLUGS={s}'
print("RAW:", cmd)
print("SPLIT:", shlex.split(cmd))
print()
PYRepository: ohcnetwork/care
Length of output: 782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the plugin resolver and the workflow context around the build args.
ast-grep outline .github/scripts/resolve_plugins.py --view expanded
printf '\n--- resolve_plugins.py ---\n'
cat -n .github/scripts/resolve_plugins.py
printf '\n--- dev Dockerfile build-arg handling ---\n'
cat -n docker/dev.Dockerfile | sed -n '1,120p'Repository: ohcnetwork/care
Length of output: 3575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the resolver to see whether the output is constrained enough to make line 51 harmless.
cat -n .github/scripts/resolve_plugins.py | sed -n '1,220p'Repository: ohcnetwork/care
Length of output: 1669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the resolver to see whether the output is constrained enough to make line 51 harmless.
cat -n .github/scripts/resolve_plugins.py | sed -n '1,220p'Repository: ohcnetwork/care
Length of output: 1669
Quote ADDITIONAL_PLUGS before passing it to docker buildx
${{ env.ADDITIONAL_PLUGS }} is JSON, and dropping it straight into the run script lets the shell strip quotes and split words, so non-empty plugin lists turn into broken --build-arg values. Move it to env: and pass "$ADDITIONAL_PLUGS" instead. PLUGIN_RESOLVED_HASH is already shell-safe, so it can stay as-is.
🧰 Tools
🪛 zizmor (1.26.1)
[info] 51-51: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 52-52: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/reusable-test.yml around lines 42 - 54, The build step in
the reusable workflow is passing ADDITIONAL_PLUGS directly from the GitHub
expression into the shell, which can strip JSON quotes and split the value
before docker buildx receives it. Move ADDITIONAL_PLUGS into the step env and
reference it as a quoted shell variable in the Build images run block, using the
existing docker buildx build invocation and keeping PLUGIN_RESOLVED_HASH
unchanged.
Source: Linters/SAST tools
| ARG PLUGIN_RESOLVED_HASH | ||
| ARG ADDITIONAL_PLUGS="" | ||
| ENV ADDITIONAL_PLUGS=$ADDITIONAL_PLUGS | ||
| RUN python3 $APP_HOME/install_plugins.py |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Same unused-ARG cache-busting gap as dev.Dockerfile.
PLUGIN_RESOLVED_HASH is declared at Line 37 but never referenced by the install_plugins.py step (Line 40), so a changed hash won't invalidate the builder-stage plugin install layer. Thread the ARG into the RUN so it actually does its job.
🔧 Suggested fix
ARG PLUGIN_RESOLVED_HASH
ARG ADDITIONAL_PLUGS=""
ENV ADDITIONAL_PLUGS=$ADDITIONAL_PLUGS
-RUN python3 $APP_HOME/install_plugins.py
+RUN PLUGIN_RESOLVED_HASH=$PLUGIN_RESOLVED_HASH python3 $APP_HOME/install_plugins.py📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG PLUGIN_RESOLVED_HASH | |
| ARG ADDITIONAL_PLUGS="" | |
| ENV ADDITIONAL_PLUGS=$ADDITIONAL_PLUGS | |
| RUN python3 $APP_HOME/install_plugins.py | |
| ARG PLUGIN_RESOLVED_HASH | |
| ARG ADDITIONAL_PLUGS="" | |
| ENV ADDITIONAL_PLUGS=$ADDITIONAL_PLUGS | |
| RUN PLUGIN_RESOLVED_HASH=$PLUGIN_RESOLVED_HASH python3 $APP_HOME/install_plugins.py |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/prod.Dockerfile` around lines 37 - 40, The builder-stage plugin
install step in docker/prod.Dockerfile is not using PLUGIN_RESOLVED_HASH, so
changes to the resolved plugin set do not invalidate the install_plugins.py
layer. Update the RUN step that invokes install_plugins.py to reference
PLUGIN_RESOLVED_HASH alongside the existing ARGs, using the same pattern as the
corresponding dev.Dockerfile cache-busting fix, so the layer is rebuilt whenever
the hash changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3696 +/- ##
========================================
Coverage 79.55% 79.56%
========================================
Files 479 479
Lines 22996 23008 +12
Branches 2378 2379 +1
========================================
+ Hits 18295 18306 +11
- Misses 4096 4098 +2
+ Partials 605 604 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/scripts/resolve_plugins.py (1)
38-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlease fail closed here instead of reusing the ref name.
This still turns a
git ls-remotefailure into a stable cache key, so branch-based plugins likepkg@mainstop invalidating the image cache when the branch moves. Catching broadExceptionmakes that even easier to miss, which is… convenient in the wrong way. Raise on resolution failure here rather than warning and continuing with the raw ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/resolve_plugins.py around lines 38 - 39, The fallback in resolve_plugins.py should not warn and continue with the raw ref name when git ls-remote fails, because that creates a stable cache key for moving branches. Update the exception handling around the git resolution logic to fail closed by raising on resolution failure instead of returning or reusing the ref name; keep the change localized near the git_url lookup and the existing except Exception as exc block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/scripts/resolve_plugins.py:
- Around line 38-39: The fallback in resolve_plugins.py should not warn and
continue with the raw ref name when git ls-remote fails, because that creates a
stable cache key for moving branches. Update the exception handling around the
git resolution logic to fail closed by raising on resolution failure instead of
returning or reusing the ref name; keep the change localized near the git_url
lookup and the existing except Exception as exc block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 513a5f1f-6cf6-43d8-ba6d-5e1a47b3d3d0
📒 Files selected for processing (1)
.github/scripts/resolve_plugins.py
|
@tellmeY18 resolve comments. Lint failing |
2 participants
Proposed Changes
actions/cache) with native BuildKit registry cache backed by GHCR (type=registry) in bothdeploy.ymlandreusable-test.ymlreproducible-containers/buildkit-cache-dancethird-party action dependencyresolve_plugins.pyscript to resolve plugin branch refs to commit SHAs viagit ls-remoteand produce a deterministic cache-busting hash as a Docker build arg (PLUGIN_RESOLVED_HASH)prune-cachejob to deletebuildcache-*GHCR package versions older than 4 weeks after each successfuldeveloppush--cache-toon fork PRs (skipped onpull_requestevents to avoid GHCR auth failures)Associated Issue
ENG-603
Summary by CodeRabbit