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

Improve ockam secure-channel create to support --from /node/NAME syntax #3205

Closed
mrinalwadhwa opened this issue Aug 12, 2022 · 8 comments
Closed

Comments

@mrinalwadhwa
Copy link
Member

Currently

> ockam node create n1
...
> ockam node create n2
...
> ockam secure-channel create --from n1 --to /node/n2/service/api

Desired

We also want to support --from /node/n1

> ockam node create n1
...
> ockam node create n2
...
> ockam secure-channel create --from /node/n1 --to /node/n2/service/api

The code for the ockam secure-channel create is:
https://github.com/build-trust/ockam/blob/develop/implementations/rust/ockam/ockam_command/src/secure_channel/create.rs

@michealkeines
Copy link
Contributor

Hi @mrinalwadhwa

I am assuming the input node name will always be in end of the protocol

Eg:
/node/n1
/node/test_node

If this is true,

sample code

image

output:

image

do you have other ways on how to approach this?

@mrinalwadhwa
Copy link
Member Author

@michealkeines thank you for looking into this!

Your approach above is a great place to start 👍
Could you turn this into a helper function and place it here

Multiple other commands have options that take NODE values and we'll want to update all of them to support the /node/n1 syntax.

I'll create a list of all the commands that need this in a separate issue shortly.

@michealkeines
Copy link
Contributor

Hi @mrinalwadhwa,

  • Created the helper function
  • Updated secure-channel create command to use the helper function

Please review my PR #3243

@mrinalwadhwa
Copy link
Member Author

@michealkeines Thank you, while I look into merging #3243.

Issues #3244, #3203, #3245, #3246, #3247 and #3248 are all the same in case you want to send a follow on PR for those.

@michealkeines
Copy link
Contributor

Sure, I will add them, separate PRs for all issues or should i add them all in one?

@mrinalwadhwa
Copy link
Member Author

In the same PR should good 👍

@mrinalwadhwa
Copy link
Member Author

Added in #3243

@mrinalwadhwa
Copy link
Member Author

@michealkeines I wanted to show your change in a demo so I ended up making all the changes I suggested up. If you're interested in picking something else, have a look at some of the new issues we posted today.

Thank you for send the above PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants