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

feat: replace address and port with URL for RPC CLI #81

Conversation

EstebanBorai
Copy link

Replaces current address and port options from the RPC option fromwafl
CLI with a single URL to use a connection string.

Fixes: #67

Replaces current `address` and `port` options from the RPC
CLI on wafl with a single URL to use a connection string.

Fixes: candlecorp#67
@EstebanBorai
Copy link
Author

Hi @fawadasaurus, I saw your issue on updating address and port for the RPC values with a single connection string and I thought it was a good opportunity to help with wasmflow development. This still in draft as I still have to spin up the RPC server and check the full usage, even though I had the CLI running locally.

Comment on lines 28 to 29
#[clap(short, long)]
pub(crate) url: Url,
Copy link
Author

Choose a reason for hiding this comment

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

Here I'm missing some validation so Im able to unwrap with confidence on L50 and L54.

format!("http://{}:{}", opts.connection.address, opts.connection.port),
format!("http://{}:{}", opts.connection.host(), opts.connection.port()),
Copy link
Author

Choose a reason for hiding this comment

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

My quick solution to allow URLs was to expose previously used address and port fields as methods.

@fawadasaurus
Copy link
Contributor

fawadasaurus commented Sep 8, 2022

Thanks @EstebanBorai. The ideal solution here would completely remove the hardcoded http://{}:{}.

What the referenced issue is asking for is to allow a string to be passed in via the CLI:

Currently, it is very rigid with just address and port and it is hardcoded to use http protocol in the invoke.rs file.

wafl rpc invoke --address=test.internal.com --port=8080 hello -- --first_name=Jane --last_name=Doe

In the future we may support many other protocols (eg: web socket) and so we would want to have the command line argument be something like:

wafl rpc invoke --uri "ws://127.0.0.1:9000" hello -- --first_name=Jane --last_name=Doe

@EstebanBorai
Copy link
Author

EstebanBorai commented Sep 8, 2022

Thanks @EstebanBorai. The ideal solution here would completely remove the hardcoded http://{}:{}.

What the referenced issue is asking for is to allow a string to be passed in via the CLI:

Currently, it is very rigid with just address and port and it is hardcoded to use http protocol in the invoke.rs file.

wafl rpc invoke --address=test.internal.com --port=8080 hello -- --first_name=Jane --last_name=Doe

In the future we may support many other protocols (eg: web socket) and so we would want to have the command line argument be something like:

wafl rpc invoke --uri "ws://127.0.0.1:9000" hello -- --first_name=Jane --last_name=Doe

Awesome! Even better then, so I will replace the current hardcoded instances of format!("http://{}... with the URL itself so we allow for protocol and the whole URL.

What do you think about using the URL crate for this? I think using it is a good idea because we get URL validation OOTB, but I would love to get your POV on using it given that as of today we are using String instead.

@fawadasaurus
Copy link
Contributor

fawadasaurus commented Sep 8, 2022

It is string right now because I just needed to unblock so it could be used in a docker/container environment :).

The default rust URI implementation is part of http::uri and so that is not ideal since we will be wanting to support many schemes. I think that having validation for URL may be valuable from an existing library.

We have a few other places where address and port are used and so we need to make sure that we keep things consistent and get the entire project standardized before we merge.

@EstebanBorai
Copy link
Author

It is string right now because I just needed to unblock so it could be used in a docker/container environment :).

The default rust URI implementation is part of http::uri and so that is not ideal since we will be wanting to support many schemes. I think that having validation for URL may be valuable from an existing library.

We have a few other places where address and port are used and so we need to make sure that we keep things consistent and get the entire project standardized before we merge.

Of course! I saw your comments following the PR linked in the issue where I got started working on this.
I also took the url crate based on the need of using different schemas and the capability of providing username/password.

Reading the docs and doing some tests here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=95d20eeace46299ee2e7d5d9234d75d2 I think it would be helpful to use it over http::Uri.

@EstebanBorai EstebanBorai deleted the feat/#67-connection-string-over-address-port branch October 14, 2022 11:57
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.

Use connection string instead of address + port.
2 participants