Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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)
Loading