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

Add min hops manipulation endpoint #780

Merged
merged 7 commits into from
Jun 3, 2021

Conversation

alexadhy
Copy link
Contributor

@alexadhy alexadhy commented May 28, 2021

Did you run make format && make check? yes

Fixes #770

Related to:

Changes:

  • Added min_hops endpoint to the http API and rpc one (@jdknives @Senyoret1 not sure about the latter, since there's route-finder)

How to test this PR:

  • run it in the integration test (using v111 config)
  • do an http call to the /visors/{pk}/min-hops endpoint (POST and GET) like:
$ curl -k https://localhost:8000/api/visors/${PK}/min-hops
$ curl -k -d '{"min_hops": 1}' -H "Content-Type: application/json" -X POST  https://localhost:8000/api/visors/${PK}/min-hops
  • or do an rpc call via skywire-cli visor route get-min-hops and skywire-cli visor route set-min-hops <some_number>

Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

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

Not sure if we should add the CLI commands just to be consistent. I cant see a use for it effectively so we may get rid of it. But also does not hurt to have it.

cmd/skywire-cli/commands/visor/routes.go Outdated Show resolved Hide resolved
@jdknives jdknives marked this pull request as ready for review May 30, 2021 22:32
@jdknives jdknives changed the title [WIP] min_hops api endpoint Add min hops manipulation endpoint May 30, 2021
Copy link
Contributor

@Senyoret1 Senyoret1 left a comment

Choose a reason for hiding this comment

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

I have 2 comments about the implementation:

  • It would be good if the GET /api/visors/{pk}/summary API endpoint returns the data, that way the UI would not have to make an additional API call, which would make the UI a bit slower.
  • Is there also a max hops configuration? If that is the case, it sould be added to the API too.

@alexadhy
Copy link
Contributor Author

alexadhy commented Jun 1, 2021

@Senyoret1:

  • first point makes sense, done.
  • currently there's no max hops configuration

Copy link
Contributor

@Senyoret1 Senyoret1 left a comment

Choose a reason for hiding this comment

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

Great, I implemented the changes in #729, thanks 👍

@jdknives jdknives merged commit be678f1 into skycoin:develop Jun 3, 2021
@alexadhy alexadhy deleted the feature/min_hops_api branch June 16, 2021 09:38
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