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

Wafl 25 issue 65 #66

Merged
merged 2 commits into from
Aug 2, 2022
Merged

Wafl 25 issue 65 #66

merged 2 commits into from
Aug 2, 2022

Conversation

fawadasaurus
Copy link
Contributor

addresses #65

@fawadasaurus fawadasaurus requested review from jsoverson and pkedy August 1, 2022 22:11
@pinkforest
Copy link

pinkforest commented Aug 2, 2022

Why not change this to url in String here as:
https://github.com/wasmflow/wasmflow/blob/main/crates/bins/wafl/src/commands/rpc/invoke.rs#L56

Seems to be hardcoded to http for some reason

String url gets into() Uri anyways from String in the end that gets passed to tonic and could avoid format!()

If the plan is to avoid certain Url schemes then there can be always assert!() and Error back

@fawadasaurus
Copy link
Contributor Author

Why not change this to url in String here as: https://github.com/wasmflow/wasmflow/blob/main/crates/bins/wafl/src/commands/rpc/invoke.rs#L56

Seems to be hardcoded to http for some reason

String url gets into() Uri anyways from String in the end that gets passed to tonic and could avoid format!()

If the plan is to avoid certain Url schemes then there can be always assert!() and Error back

This is an interesting comment. For me, part of wasmflow is to simplify (abstract away) these decisions from the developer so that wasmflow can evolve without breaking people's code. Part of the philosophy behind making Wasmflow was that developer already have too many choices in the implementation details and instead should just be able to focus on delivering value with fewer decisions.

Yes, today the RPC call may be using HTTP but in the future it would be possible to switch to RPC over NATS or other type of event queue. Maybe even a web socket or some type of persistent connection mechanism could be used. In the future, there could perhaps be a performance based best alternative or fall-back capabilities to use different protocols.

@pinkforest
Copy link

pinkforest commented Aug 2, 2022

Most protocols that are worthwhile today take url based endpoint and as a developer I just want to throw url in without having to figure out command line options - throwing url at something makes me not need make decisions how to pass things to separate CLI arguments - URLs often also carry other information in parameters.

You can always in the future add further CLI options for protocols that require different scheme but URL should be the default that covers the most.

Most MQs including nats have URL scheme: https://docs.nats.io/using-nats/developer/connecting

Even WebSockets are modelled as ws[s]:// and mqtt mqtt[s]:// as well etc ..

And yes URL as a tried and trusted standardised scheme supports multiple protocols, encryption / authentication options, connection options as well as other supplied often proprietary protocol / application specific arguments at ease

@fawadasaurus
Copy link
Contributor Author

Ok. I am going to make this a new issue because we also want to make sure this is consistent with wasmflow binary and manifest and introduces no issues end to end.

@jsoverson
Copy link
Contributor

LGTM. We do need to fix the argument to make it a connection string, but we'll have to do that across the config and all the binaries to keep everything in sync. Tracking in #67

@jsoverson jsoverson merged commit 8f29c7e into main Aug 2, 2022
@jsoverson jsoverson deleted the WAFL-25-issue-65 branch August 2, 2022 16:21
pkedy added a commit that referenced this pull request Jan 9, 2023
* Stubbed out docsite structure.

* Fixup
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.

3 participants