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

wasm: support http(s) fetch of Wasm files #3005

Merged
merged 5 commits into from
Jul 27, 2023

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented Jul 21, 2023

Signed-off-by: Edoardo Vacchi [email protected]

Description

We add support to simple HTTP(S) fetch of (optionally compressed) Wasm files; i.e. not tarballs, but compressed responses from an HTTP(S) server. Will update docs later, opening draft in the meantime, for early feedback.

Issue reference

This addresses issue #2700 without closing, because we don't support tar.gz files yet.

Checklist

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

Optionally, the response may compressed

Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
Copy link
Contributor Author

evacchi commented Jul 21, 2023

/cc @codefromthecrypt FYI

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Jul 21, 2023 via email

@evacchi evacchi marked this pull request as ready for review July 24, 2023 09:07
@evacchi evacchi requested review from a team as code owners July 24, 2023 09:07
@Taction
Copy link
Member

Taction commented Jul 24, 2023

Thanks! LGTM overall.
Newly added files missed the copyright header. Also, please fix the lint error, you can find the error here or run make lint in the root dictionary https://github.com/dapr/components-contrib/actions/runs/5623856267/job/15286217988?pr=3005.
This is the header:

/*
Copyright 2023 The Dapr Authors
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
    http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implieout.
See the License for the specific language governing permissions and
limitations under the License.
*/

@evacchi
Copy link
Contributor Author

evacchi commented Jul 24, 2023

thanks, I have also renamed httpfetcher.go -> http.go and httpFetcher.fetch to httpClient.get

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally didn't submit my reivew. Overall LGTM but some comments

internal/wasm/http.go Outdated Show resolved Hide resolved
internal/wasm/http.go Show resolved Hide resolved
internal/wasm/http.go Outdated Show resolved Hide resolved
internal/wasm/wasm.go Outdated Show resolved Hide resolved
internal/wasm/wasm.go Show resolved Hide resolved
Signed-off-by: Edoardo Vacchi <[email protected]>
bindings/wasm/output.go Outdated Show resolved Hide resolved
bindings/wasm/output_test.go Outdated Show resolved Hide resolved
internal/wasm/http.go Outdated Show resolved Hide resolved
internal/wasm/http.go Outdated Show resolved Hide resolved
Signed-off-by: Alessandro (Ale) Segala <[email protected]>
ItalyPaleAle
ItalyPaleAle previously approved these changes Jul 26, 2023
@ItalyPaleAle
Copy link
Contributor

Thanks! Will merge when tests pass

auto-merge was automatically disabled July 27, 2023 08:02

Head branch was pushed to by a user without write access

@evacchi
Copy link
Contributor Author

evacchi commented Jul 27, 2023

some changes were committed by mistake (arg handling), those I may revisit separately in another PR. Thanks for your patience @ItalyPaleAle

@ItalyPaleAle ItalyPaleAle added this pull request to the merge queue Jul 27, 2023
Merged via the queue into dapr:master with commit 13dfbdb Jul 27, 2023
163 of 167 checks passed
@evacchi evacchi deleted the wasm-http-fetch branch July 27, 2023 15:37
evacchi added a commit to evacchi/kube-scheduler-wasm-extension that referenced this pull request Jul 28, 2023
Extends the current plugin config to use instead a URI. In the case
of `file://` the behavior is the same as it is currently.
In the case of `http(s)://` it will fetch the URI and try to
evaluate it as a wasm payload.

This PR is based on earlier work on `dapr/component-contrib`.
See: dapr/components-contrib#3005

Signed-off-by: Edoardo Vacchi <[email protected]>
evacchi added a commit to evacchi/kube-scheduler-wasm-extension that referenced this pull request Jul 28, 2023
Extends the current plugin config to use instead a URI. In the case
of `file://` the behavior is the same as it is currently.
In the case of `http(s)://` it will fetch the URI and try to
evaluate it as a wasm payload.

This PR is based on earlier work on `dapr/component-contrib`.
See: dapr/components-contrib#3005

Signed-off-by: Edoardo Vacchi <[email protected]>
evacchi added a commit to evacchi/kube-scheduler-wasm-extension that referenced this pull request Jul 28, 2023
Extends the current plugin config to use instead a URI. In the case
of `file://` the behavior is the same as it is currently.
In the case of `http(s)://` it will fetch the URI and try to
evaluate it as a wasm payload.

This PR is based on earlier work on `dapr/component-contrib`.
See: dapr/components-contrib#3005

Signed-off-by: Edoardo Vacchi <[email protected]>
evacchi added a commit to evacchi/kube-scheduler-wasm-extension that referenced this pull request Jul 28, 2023
Extends the current plugin config to use instead a URI. In the case
of `file://` the behavior is the same as it is currently.
In the case of `http(s)://` it will fetch the URI and try to
evaluate it as a wasm payload.

This PR is based on earlier work on `dapr/component-contrib`.
See: dapr/components-contrib#3005

Signed-off-by: Edoardo Vacchi <[email protected]>
@ItalyPaleAle ItalyPaleAle added this to the v1.12 milestone Aug 2, 2023
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.

4 participants