-
Notifications
You must be signed in to change notification settings - Fork 630
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
Add support for webassembly_binding in cloudflare_worker_script #780
Conversation
I found how to run acceptance tests, the worker does properly contain the 15 byte wasm module, as expected:
|
Should I update CHANGELOG.md? |
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
"base64": { |
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 find the schema name here a little lacking; what are your thoughts on using something more aligned with the existing schema fields but still convey it's base64 encoded, like base64_encoded_text
? We could also reuse text
here and note in the documentation it is the bas64 encoded value?
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.
Hm, I'm not particularly happy with the notion of "text" either, what's actually supposed to be here is a wasm module, which is binary data. module
probably works best, changing it.
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.
How about data
? module
leaves some ambiguity for a novice like me.
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.
It feels like data
would be more ambiguous than module
, the latter being defined in spec to be this exact thing
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.
Is this still a point of contention? I can deal with it being data data
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.
No point of contention from me, just want this make sense to those that are going to be using it. I have little experience here so happy to leave as-is for your wording.
Thanks for the quick turn around @ldesgoui! No need to update the CHANGELOG, we'll drop that in once it lands to avoid resolving merge conflicts for it within your branch. |
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.
integration suite is all green 🍏 thanks for this!
…loudflare#780) Bumps [goreleaser/goreleaser-action](https://github.com/goreleaser/goreleaser-action) from 2.8.0 to 2.8.1. - [Release notes](https://github.com/goreleaser/goreleaser-action/releases) - [Commits](goreleaser/goreleaser-action@v2.8.0...v2.8.1) --- updated-dependencies: - dependency-name: goreleaser/goreleaser-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This remains a draft for now because I have not tested beyond running
make test
, I will attempt to make it run with terraform, I am quite new with the tool, so it may take some time.Closes #178
cc @jacobbednarz