-
Notifications
You must be signed in to change notification settings - Fork 27
feat: enable GH action caching #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
init cache Update setup.js
|
GH checks are green https://github.com/frichtarik/setup-sam/pull/1/checks |
|
Ola, any chance on getting a review ? :) |
bnusunny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! Cross-runner caching is a great improvement.
One concern: the cache key doesn't include the runner OS image version:
const cacheKey = `sam-cli-${os.platform()}-${arch}-${version}`;
Since @actions/cache persists across workflow runs, this could cause issues when users run on different runner images. SAM CLI bundles libraries like OpenSSL in dist/_internal/, and when it spawns subprocesses (npm, node, etc.), those processes can pick up the bundled libraries instead of system ones. If the bundled library version doesn't match what the system tools expect, builds fail — see aws/aws-sam-cli#8542 for an example where Node.js fails because it loads SAM CLI's older OpenSSL instead of the system version.
Scenario:
- User caches SAM CLI on
ubuntu-22.04 - GitHub updates
ubuntu-latesttoubuntu-24.04(or user changes their workflow) - Cached SAM CLI's bundled libraries are incompatible with the new runner's system tools
Suggested fix — include the runner image in the cache key:
const imageOS = process.env.ImageOS || "unknown";
const cacheKey = `sam-cli-${os.platform()}-${imageOS}-${arch}-${version}`;
This would produce keys like sam-cli-linux-ubuntu22-x86_64-1.139.0, ensuring cache isolation between different runner images.
Hi @bnusunny, thank you very much for the review. Of course you are right, I've adjusted the code and tests accordingly. Fresh test workflow results can be found here Looking forward to finishing this. F. |
|
@frichtarik Thanks for the update. Looks good. There is one minor lint error. Once it is fixed, we can merge it. |
|
@bnusunny I've missed new commit in upstream, now everything should pass. |
Which issue(s) does this change fix?
#103
Description
main goal:
side tasks:
Checklist
npm run allBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 License.