Skip to content
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

Changes wasm middleware and output binding config to url instead of path #2817

Merged
merged 5 commits into from
May 2, 2023

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Apr 28, 2023

Description

This switches the wasm middleware and output binding config to use url instead of path to read the WebAssembly module. This allows us to later implement HTTP and OCI.

Note: The output binding passes STDIN as well as arguments. This means it can run intepreters such as python. I will be implementing this with the OCI scheme, tested with https://github.com/orgs/vmware-labs/packages/container/package/python-wasm

Issue reference

related to, but won't close #2700

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@codefromthecrypt codefromthecrypt requested review from a team as code owners April 28, 2023 09:48
yaron2
yaron2 previously approved these changes Apr 28, 2023
@yaron2 yaron2 dismissed their stale review April 28, 2023 12:43

Build failure

@yaron2
Copy link
Member

yaron2 commented Apr 28, 2023

Thanks, LGTM overall. See lint failure.

@codefromthecrypt codefromthecrypt force-pushed the output-url branch 2 times, most recently from f391b81 to aea6892 Compare April 28, 2023 23:07
@codefromthecrypt
Copy link
Contributor Author

thanks for the look. lint fixed now!

This switches the wasm middleware and output binding config to use url
instead of path to read the WebAssembly module. This allows us to later
implement file, http and OCI.

Note: The output binding passes STDIN as well as arguments. This means
it can run intepreters such as python. I will be implementing this with
the OCI scheme, tested with https://github.com/orgs/vmware-labs/packages/container/package/python-wasm

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt changed the title Changes wasm output binding config to url instead of path Changes wasm middleware and output binding config to url instead of path Apr 30, 2023
codefromthecrypt pushed a commit to codefromthecrypt/samples that referenced this pull request Apr 30, 2023
The configuration property for the wasm file is renamed per dapr/components-contrib#2817

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Apr 30, 2023

@yaron2 @salaboy I noticed 1.11 cutoff is tomorrow, so made a decision to just normalize the config property to "url" for both wasm middleware and the output binding and not implement HTTP, yet. Basically, this is ready to implement "http://" "https://" and "oci://" schemes.

middleware:

output binding:

  • we need to merge this, so that I can update dapr/dapr go.mod and actually wire it up. There's WASM output binding content docs#3358 to document, but that doesn't make sense until it is integrated. I can do this as soon as first thing tomorrow asia time, depending on when this lands.

@yaron2
Copy link
Member

yaron2 commented May 1, 2023

Is this a breaking change in terms of the path -> URL change?

@codefromthecrypt
Copy link
Contributor Author

Is this a breaking change in terms of the path -> URL change?

yes, it is unless we want to back port the old one. As it is an alpha component and we broke it before not sure the rules.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

ps I was wrong, it looks like the wasm binding was already integrated in dapr/dapr#6269 so the only thing left is docs and just updating components-contrib version in dapr/dapr after merge

@yaron2
Copy link
Member

yaron2 commented May 1, 2023

Is this a breaking change in terms of the path -> URL change?

yes, it is unless we want to back port the old one. As it is an alpha component and we broke it before not sure the rules.

We can technically break the spec as this is an Alpha component, but where possible we try to provide backward compatibility. For this early component I don't think it's required.

Have you tested the middleware locally with Dapr after these changes?

@codefromthecrypt
Copy link
Contributor Author

@yaron2 thx for understanding. I hadn't noticed a lot of use of middleware, so gut feel was to change the url before it becomes permanent confusion. I'll try middleware with a local build now and report back.

p.s. dapr/docs#3362 for the docs on components.

@codefromthecrypt
Copy link
Contributor Author

@yaron2 tested and captured output here

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

I added a commit to simplify the wasm output binding invocation, as well tighten the benchmark. We no longer return exit code zero on success (since 1.0)

@berndverst
Copy link
Member

berndverst commented May 1, 2023

@yaron2 @salaboy I noticed 1.11 cutoff is tomorrow

@codefromthecrypt keep in mind the following deadlines in the future:

  1. Feature Freeze -- any significant features should have complete PRs opened before this date (does not necessarily apply to this PR).
  2. Code Freeze -- PRs need to have been fully reviewed and merged by this date. This is not the date for opening new PRs. It's best to allow 1-2 weeks before code freeze to give maintainers time to provide feedback and time for you to respond to feedback etc.

Of course we'll try to get this PR reviewed and merged -- but do keep these dates in mind in the future to make things easier for us maintainers. Thanks for your understanding.

@codefromthecrypt
Copy link
Contributor Author

@berndverst absolutely will bear in mind dates.. in fact I would have prioritized differently if I knew them. Where generally do I find these as I found out by accident of asking nervously.

@yaron2 yaron2 added this pull request to the merge queue May 2, 2023
Merged via the queue into dapr:master with commit d22c4bc May 2, 2023
@berndverst
Copy link
Member

@berndverst absolutely will bear in mind dates.. in fact I would have prioritized differently if I knew them. Where generally do I find these as I found out by accident of asking nervously.

We always create an issue in dapr/dapr that we also pin in the issues list there called "[version] Release Planning", for example "v1.11 Release Planning" dapr/dapr#5966

That's where we keep the timeline up to date and also discuss the major features we are including in that release.

Soon after we ship v1.11 we will create another such issue and pin it - so you can look for the "v1.12 Release Planning" issue then. The issue will be kept up to date with timelines.

@codefromthecrypt codefromthecrypt deleted the output-url branch May 2, 2023 01:46
@ItalyPaleAle ItalyPaleAle added this to the v1.11 milestone May 25, 2023
aaguilartablada pushed a commit to aaguilartablada/dapr-components-contrib that referenced this pull request Jun 1, 2023
…ath (dapr#2817)

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
Signed-off-by: Alvaro Aguilar <alvaro.aguilar@scrm.lidl>
yaron2 pushed a commit to dapr/samples that referenced this pull request Jul 31, 2023
* component http wasm: adjust for config drift

The configuration property for the wasm file is renamed per dapr/components-contrib#2817

Signed-off-by: Adrian Cole <adrian@tetrate.io>

* fuzz

Signed-off-by: Adrian Cole <adrian@tetrate.io>

---------

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Vixen0219 added a commit to Vixen0219/samples that referenced this pull request May 6, 2024
* component http wasm: adjust for config drift

The configuration property for the wasm file is renamed per dapr/components-contrib#2817

Signed-off-by: Adrian Cole <adrian@tetrate.io>

* fuzz

Signed-off-by: Adrian Cole <adrian@tetrate.io>

---------

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support WASM files being fetched from an http or https endpoint
5 participants