Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/actions/s3-layer-cache-settings/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,6 @@ runs:
cacheToManifestNames.push('${{ inputs.name }}');
}

core.setOutput('cache_to', `${settingsString},mode=${{ inputs.mode }},name=${cacheToManifestNames.join(';')}`);
// Cache export is an optimization, not a build/publication gate.
// Preserve build and registry failures; only tolerate cache upload outages.
core.setOutput('cache_to', `${settingsString},mode=${{ inputs.mode }},name=${cacheToManifestNames.join(';')},ignore-error=true`);
53 changes: 53 additions & 0 deletions .github/scripts/tests/test_s3_layer_cache.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
"""Execute the cache settings action with synthetic, credential-free inputs."""
import json
from pathlib import Path
import subprocess
import textwrap
import unittest

ROOT = Path(__file__).resolve().parents[3]


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)
Comment on lines +11 to +25

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return json.loads(result)

def test_should_tolerate_only_export_errors_and_preserve_imports(self):
settings = self.settings()
self.assertTrue(settings['cache_to'].endswith(',ignore-error=true'))
self.assertEqual(settings['cache_to'].count('ignore-error='), 1)
imports = settings['cache_from'].splitlines()
self.assertEqual(len(imports), 3)
for item in imports:
self.assertNotIn('ignore-error', item)
self.assertNotIn('mode=', item)
self.assertIn('prefix=cache-layers/linux/amd64/', item)
self.assertIn('endpoint_url=https://cache.example.invalid', item)
expected = 'dashmate-helper_sha_' + 'a' * 40 + ';dashmate-helper_tag_refs-pull-42-merge'
self.assertIn('name=' + expected + ',ignore-error=true', settings['cache_to'])

def test_should_keep_shared_manifest_writes_explicitly_opt_in(self):
self.assertNotIn(';dashmate-helper,', self.settings()['cache_to'])
self.assertIn(';dashmate-helper,ignore-error=true', self.settings(shared=True)['cache_to'])
self.assertNotIn('endpoint_url=', self.settings(endpoint='')['cache_to'])

def test_should_not_ignore_build_or_registry_publication_failures(self):
action = (ROOT / '.github/actions/docker/action.yaml').read_text()
build = action.split(' - name: Build and push Docker image', 1)[1]
self.assertNotIn('continue-on-error', build)
self.assertNotIn('ignore-error', build)
self.assertIn('cache-to: ${{ steps.layer_cache_settings.outputs.cache_to }}', build)
self.assertIn('name-canonical=true,push=true', build)
26 changes: 26 additions & 0 deletions .github/workflows/cache-action-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: Cache action regression tests

on:
pull_request:
paths:
- '.github/actions/s3-layer-cache-settings/**'
- '.github/actions/docker/**'
- '.github/scripts/tests/test_s3_layer_cache.py'
- '.github/workflows/cache-action-tests.yml'

permissions:
contents: read

jobs:
cache-action-tests:
runs-on: ubuntu-24.04
timeout-minutes: 5
steps:
- uses: actions/checkout@v4
with:
persist-credentials: false
- uses: actions/setup-node@v4
with:
node-version: '22'
- name: Test optional cache export and fatal publication boundaries
run: python3 -m unittest discover -s .github/scripts/tests -p 'test_s3_layer_cache.py' -v
Loading