-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: unstableNetlifyFunctionsSupport.includeDirs configuration #182
Conversation
unclear about that ubuntu 10.13 failure |
console.log(`Added ${includes} to ${zipName}`) | ||
} | ||
}) | ||
zip.writeZip(zipName) |
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.
Note: we might want to remove the mtime
when creating the archive.
Otherwise, every zip archive of the same Function will have a different checksum. This has created some issues in the past which ended up increasing our AWS Lambda usage weekly rate significantly.
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.
hmm i’ve been reading the adm-zip docs (that are v unclear lol) and i dont quite understand how i’d do this 😐 do yk exactly how this would be done with this package or should i switch to whatever zisi is doing?
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.
From checking their source code, it does not look like adm-zip
allows setting the mtime
.
Based on this, I think copying the logic from zip-it-and-ship-it
might be good. Eventually, I think we might want to fix this in zip-it-and-ship-it
so unstableNetlifyFunctionsSupport
should hopefully be removed in the future? (cc @eduardoboucas) If so, duplicating the logic might be ok IMHO.
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.
yes this will be removed in the future! :D
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.
Maybe this specific part can be a separate PR since it's a lower visibility issue?
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.
i'm gonna defer the archiver/adm swap for if issues arise - as of right now there will be v few users using this and merging this is mainly to keep the temporary logic/branch up to date with main. want to unblock the cache PR and a lot of the other work i have to do and promised this to another user 😢 @eduardoboucas would you mind opening and/or sharing the issue to track the toml-side version of this work? 🙏 🙏
19da168
to
e8248e3
Compare
e8248e3
to
d4640c8
Compare
d4640c8
to
8286365
Compare
* chore: repro ENAMETOOLONG * refactor: extract encodeBlobKey function from build and handlers into shared impl * fix: truncate long key names * fix: lower max length * fix: use base64url to prevent clashes with blobstore urls * use filename short enough to be writable by next.js, but long enough to trigger ENAMETOOLONG once base64'd * fix a bug and add test * fix: fix test * fix: remove revalidation from test * fix: use .cjs file so that vitest doesn't stumble * chore: supress TS error * fix: update cache-handler test * fix: also move to base64+url in test suite * fix: use encodeBlobKey across full repo * fix: let's try .ts file again * same fix for static.test.ts * fix: use webcrypto --------- Co-authored-by: Rob Stanford <me@robstanford.com>
unstableNetlifyFunctionsSupport
i think should be fine?functions
was v unclear and this is just gonna be taken out anyways and prob only communicated to people who find the open issue so 🤷