-
Notifications
You must be signed in to change notification settings - Fork 188
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
Rename server
to innernet-server
, expose as a library
#320
Conversation
@strohel Hi! Have a couple of questions regarding this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just one little meta-nit.
For the record I'm looking at the changes locally using
git fetch origin pull/320/head:extract-server-to-lib
git diff --find-copies main...extract-server-to-lib
which is able to detect the file move.
To answer your questions:
- Would it makes sense to introduce
Server
struct, so that it won't be necessary toopen_database_connection
every time user uses any of server commands? It won't really change anything for a CLI, but will make it more convenient for library usage.
Yeah it would make sense, though I'd prefer for that to be done in a separate PR on top of this one (to be able so see the actual diff) - reserving this one to be just the move.
- What would be library name,
innernet_server
?
Yeah perhaps, maybe innernet-server
for consistency. That's already used for the binary name, but that shuoldn't conflict?
- Other than docs, is there anything else I should cover?
Nothing I could think of, maybe see if any part of the README can be updated.
I see the Docker tests CI fails, but if also fails when I ran in ton main
now: https://github.com/tonarino/innernet/actions/runs/10316957718
It would be great to fix that separately and merge to main before this one.
I'd also love if either @bschwind or @mcginty could review 🙏.
Hi, thanks for a review!
Unforunately, UPDATE: Looks like there are crates with hyphens, is it ok if I change package name to
Doesn't git marks it automatically based on content or should I somehow assist here? |
I've changed package's name to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sqrtsanta, sorry for the delay getting back to you!
One little suggestion (sorry only noticed now even though the code was also in your previous version), plus I'd still like to see a review from @bschwind or @mcginty. Or perhaps if you have some hints on the Docker CI failures on main
? 🙏
I've changed package's name to
innernet-server
.
That's good! Also a nice preparation towards #11
Unfortunately had to change installation instructions due to that and that made inconsistency between client & server:
I think this is fine. We may (have to) rename client
to innernet-client
later anyway.
Thanks @strohel! Docker CI failures are due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is great! Sorry I'm so late in reviewing it. I don't have much to add that @strohel hasn't already mentioned so just throwing my +1 in here.
Seems like we got a new clippy lint triggering after a toolchain upgrade. We could fix that here or in a separate PR to main. The dockerfile build will still fail either way though :(
Thanks to great pointer by @sqrtsanta in #320 (comment)
Thanks to great pointer by @sqrtsanta in #320 (comment)
Thanks to great pointer by @sqrtsanta in #320 (comment)
@sqrtsanta we've finally fixed the Docker tests issue (thanks for the hint!) and a new clippy issues that popped in the mean time. Please rebase your PR on |
@strohel Done, should be good now. |
server
to innernet-server
, expose as a library
server
to innernet-server
, expose as a libraryserver
to innernet-server
, expose as a library
Perfect! I went through to code again, everything looks good, merging! 🥳 Thank you again @sqrtsanta for your contribution. 🙇 |
This extracts commands from innernet-server binary so that it will be possible to use it as a library. This way it will be easier to make own wrappers around innernet-server e.g. gprc/http server.
It exports certain commands:
add_cidr
rename_cidr
delete_cidr
add_peer
rename_peer
enable_or_disable_peer
serve
uninstall
It exports
initialize::init_wizard
as well, although my guess is this command will be used via CLI only.