Repository navigation
ci: tolerate optional S3 cache export outages on chore/bump-rust-dashcore-secp-033 - #5188
ktechmidas wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 9d86c41) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The production change correctly limits error tolerance to S3 cache export, preserves cache imports and fatal build/publication failures, and exactly matches the two-file upstream repair. All three regression tests and git diff --check pass at ec0e27c. One non-blocking gap remains: this branch's workflows do not execute the newly added tests.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The two-file change narrowly makes optional S3 cache exports nonfatal with ignore-error=true and adds focused regression tests, leaving build, test, publication, and cache-import behavior unchanged. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 81% left, weekly 86% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/scripts/tests/test_s3_layer_cache.py`:
- [SUGGESTION] .github/scripts/tests/test_s3_layer_cache.py:11-25: New regression tests are not executed by any CI workflow on this branch
These tests execute the actual cache-action script and pass locally, but no workflow at this head discovers `.github/scripts/tests/`. The existing Python discovery commands target `packages/swift-sdk/scripts` and the unrelated root `scripts/tests` directory. Upstream already had discovery of `.github/scripts/tests` before #5165; this target branch does not, so copying the test file alone leaves these new regressions unenforced in CI. Add a lightweight workflow step running `python3 -m unittest discover -s .github/scripts/tests -v` with Node available, and ensure changes to the cache action and its tests trigger it. This does not affect the correctness of the cache-export fix.
| class LayerCacheTests(unittest.TestCase): | ||
| def settings(self, shared=False, endpoint='https://cache.example.invalid'): | ||
| action = (ROOT / '.github/actions/s3-layer-cache-settings/action.yaml').read_text() | ||
| script = textwrap.dedent(action.split(' script: |\n', 1)[1]) | ||
| values = {'inputs.region': 'test-region', 'inputs.bucket': 'test-bucket', | ||
| 'inputs.prefix': 'cache-layers/linux/amd64/', 'inputs.endpoint': endpoint, | ||
| 'inputs.head_ref': 'refs/pull/42/merge', 'inputs.name': 'dashmate-helper', | ||
| 'github.sha': 'a' * 40, 'inputs.mode': 'max', | ||
| 'inputs.cache_to_name': str(shared).lower()} | ||
| for key, value in values.items(): | ||
| script = script.replace('${{ ' + key + ' }}', value) | ||
| self.assertNotIn('${{', script) | ||
| harness = 'const output = {}; const core = {setOutput: (k, v) => output[k] = v};\n' | ||
| result = subprocess.check_output(['node', '-e', harness + script | ||
| + '\nconsole.log(JSON.stringify(output));'], text=True) |
There was a problem hiding this comment.
🟡 Suggestion: New regression tests are not executed by any CI workflow on this branch
These tests execute the actual cache-action script and pass locally, but no workflow at this head discovers .github/scripts/tests/. The existing Python discovery commands target packages/swift-sdk/scripts and the unrelated root scripts/tests directory. Upstream already had discovery of .github/scripts/tests before #5165; this target branch does not, so copying the test file alone leaves these new regressions unenforced in CI. Add a lightweight workflow step running python3 -m unittest discover -s .github/scripts/tests -v with Node available, and ensure changes to the cache action and its tests trigger it. This does not affect the correctness of the cache-export fix.
source: glm-5.3-flash (phase1-reviewer: general)
There was a problem hiding this comment.
Addressed by infraclaw in 9d86c41094490a41e0dc2e5083cea5de7b8a3fcb, fast-forwarded onto this existing infrastructure branch (no replacement PR). Added only .github/workflows/cache-action-tests.yml: a read-only, five-minute GitHub-hosted job with Node 22 and credential persistence disabled. It triggers for both cache/Docker action paths, the unchanged cache regression file, and its own workflow, and runs the existing three tests directly. This does not depend on pending #5167 and continues covering action-only changes after that PR lands.
All three tests pass locally; eight path-filter inclusion/exclusion checks and YAML/diff gates passed. More importantly, actual PR-attributable run 36564342784, attempt 1 completed successfully on this exact head at 11:51 UTC, including the cache regression step. Cache behavior, application code/tests, manifests and self-hosted workers are unchanged.
This is repair evidence, not self-approval or target adoption. New-head independent reviews and ordinary protected merge gates remain required; the finding/thread has not been dismissed or resolved by me. Protected account attribution is transport, not latte's personal review.
There was a problem hiding this comment.
Resolved (re-reviewed at 9d86c410): The added .github/workflows/cache-action-tests.yml now runs the cache regression tests with Node 22 on a hosted runner and triggers for the relevant cache actions, test file, and workflow changes. The three tests pass against the exact reviewed head.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The pull request correctly limits BuildKit tolerance to optional S3 layer-cache exports by adding ignore-error=true only to cache-to. Cache imports, Docker builds, and registry publication remain fatal, and the added hosted workflow executes the three regression tests on relevant changes. No in-scope defects were found.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff makes a single, well-contained cache-export behavior change by adding ignore-error=true, with straightforward regression tests and a hosted CI workflow, without changing any critical surface. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— architecture-layering (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 86% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Issue being fixed or feature implemented
Backport the merged CI-only optional S3 cache-export repair #5165 to
chore/bump-rust-dashcore-secp-033. An unavailable optimization must not invalidate a successful build or image publication.What was done?
5f4ab6c2f98cf3c01f35a5ffa81f61957a4cd50fonto targete2fd47db5cf2c299f0bf1a0eddda60688f2fd8a5.ignore-error=true). Imports, architecture prefixes and explicit shared-writer opt-in remain unchanged.How Has This Been Tested?
git diff --checkpass; every non-.githubroot-tree object equals the exact target.Author recovery guidance (after merge only)
If your exact failed step is optional S3 cache export, update your own PR from
chore/bump-rust-dashcore-secp-033after this repair lands and let the new head start CI. Re-running an unchanged old head does not adopt this action repair. This does not fix Android/sudo setup faults or application/test failures; those remain separate.Breaking Changes
None. Existing image eligibility and legacy runner capacity are preserved.
Checklist
Authored and operated by infraclaw under latte's scoped CI authorization. Protected transport account attribution is not its owner's personal review or approval. No bypass or self-approval is requested.