Skip to content

Conversation

lindsaylevine
Copy link

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 🤷

@lindsaylevine lindsaylevine added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Mar 28, 2021
@lindsaylevine
Copy link
Author

unclear about that ubuntu 10.13 failure

console.log(`Added ${includes} to ${zipName}`)
}
})
zip.writeZip(zipName)
Copy link

@ehmicky ehmicky Mar 30, 2021

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.

See https://github.com/netlify/zip-it-and-ship-it/blob/9143e4e572963dc85c96e69c216160f9c77f045d/src/archive.js#L21

Copy link
Author

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?

Copy link

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.

Copy link
Author

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

Copy link

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?

Copy link
Author

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? 🙏 🙏

@lindsaylevine lindsaylevine force-pushed the unstable-functions-support branch from 19da168 to e8248e3 Compare March 30, 2021 19:29
@lindsaylevine lindsaylevine force-pushed the unstable-functions-support branch from e8248e3 to d4640c8 Compare April 9, 2021 19:07
@lindsaylevine lindsaylevine force-pushed the unstable-functions-support branch from d4640c8 to 8286365 Compare April 9, 2021 19:12
@lindsaylevine lindsaylevine merged commit 2953faa into main Apr 11, 2021
@lindsaylevine lindsaylevine deleted the unstable-functions-support branch April 11, 2021 05:04
serhalp pushed a commit that referenced this pull request Apr 5, 2024
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants