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

Feature/add min hops config #656

Merged

Conversation

i-hate-nicknames
Copy link
Contributor

Did you run make format && make check?

Fixes #319

Changes:

  • Add min_hops visor router config option

How to test this PR:

@i-hate-nicknames i-hate-nicknames changed the base branch from master to develop January 14, 2021 13:37
Copy link
Contributor

@Darkren Darkren left a comment

Choose a reason for hiding this comment

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

Great job!

@jdknives
Copy link
Member

@i-hate-nicknames since this is changing the config of a visor, we need to think about whether we need to add this to the config migration and whether we need to bump the config version.

@i-hate-nicknames
Copy link
Contributor Author

@jdknives I'm not sure if migration is needed, and what is the procedure. I need to research it.

As of now, I can't think of issues that can be introduced by this change.

If new code reads old config format, the values are not there and config struct gets zero values. Zero value of minHops is the default value, so no problem there. Zero value of maxHops is a problem, but it is reset to default value (50) here for such case.

If old code reads new config file, the values will be ignored (no change in the structure is made, only new fields are added).

@i-hate-nicknames i-hate-nicknames force-pushed the feature/add-min-hops-config branch from e2e173a to 8bd07e2 Compare March 18, 2021 13:00
@jdknives jdknives merged commit 5b4e24a into skycoin:develop Mar 18, 2021
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.

Allow to set min hops for route requests
3 participants