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

Rename server to innernet-server, expose as a library #320

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Rename server to innernet-server, expose as a library #320

merged 1 commit into from
Sep 13, 2024

Conversation

sqrtsanta
Copy link
Contributor

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.

@sqrtsanta
Copy link
Contributor Author

@strohel Hi! Have a couple of questions regarding this PR:

  1. Would it makes sense to introduce Server struct, so that it won't be necessary to open_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.
  2. What would be library name, innernet_server?
  3. Other than docs, is there anything else I should cover?

Copy link
Member

@strohel strohel left a 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:

  1. Would it makes sense to introduce Server struct, so that it won't be necessary to open_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.

  1. 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?

  1. 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 🙏.

server/src/lib.rs Outdated Show resolved Hide resolved
@sqrtsanta
Copy link
Contributor Author

sqrtsanta commented Aug 28, 2024

Hi, thanks for a review!

Yeah perhaps, maybe innernet-server for consistency. That's already used for the binary name, but that shuoldn't conflict?

Unforunately, innernet-server as a library name is not possible due to library target names cannot contain hyphens. Also, right now package name is server. Should I change it too? I can leave it as server and add lib.name as innernet_server, but I suspect it will be a little confusing if they won't match.

UPDATE: Looks like there are crates with hyphens, is it ok if I change package name to innernet-server then?

which is able to detect the file move.

Doesn't git marks it automatically based on content or should I somehow assist here?

@sqrtsanta
Copy link
Contributor Author

sqrtsanta commented Aug 29, 2024

I've changed package's name to innernet-server. Unfortunately had to change installation instructions due to that and that made inconsistency between client & server:

cargo install --git https://github.com/tonarino/innernet --tag v1.6.1 client
cargo install --git https://github.com/tonarino/innernet --tag v1.6.1 innernet-server

Copy link
Member

@strohel strohel left a 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.

server/src/main.rs Outdated Show resolved Hide resolved
@sqrtsanta
Copy link
Contributor Author

Thanks @strohel! Docker CI failures are due to golang.org/x/net/internal/socket & Go1.23. Here bettercap/bettercap#1106 folks stumbled upon the same problem. We either downgrade to Go1.22 or wait until wireguard-go bumps x/net.

Copy link
Member

@bschwind bschwind left a 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 :(

strohel added a commit that referenced this pull request Sep 13, 2024
strohel added a commit that referenced this pull request Sep 13, 2024
strohel added a commit that referenced this pull request Sep 13, 2024
@strohel
Copy link
Member

strohel commented Sep 13, 2024

@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 main, then the CI should go green again. Feel free squash your PR to a single commit first, as that may help with the rebase (I touched main.rs in the clippy PR).

@sqrtsanta
Copy link
Contributor Author

@strohel Done, should be good now.

@strohel strohel changed the title Expose innernet-server as a library Ranema server to innernet-server, expose as a library Sep 13, 2024
@sqrtsanta sqrtsanta changed the title Ranema server to innernet-server, expose as a library Rename server to innernet-server, expose as a library Sep 13, 2024
@strohel
Copy link
Member

strohel commented Sep 13, 2024

Perfect! I went through to code again, everything looks good, merging! 🥳

Thank you again @sqrtsanta for your contribution. 🙇

@strohel strohel merged commit 9578a15 into tonarino:main Sep 13, 2024
5 checks passed
@sqrtsanta sqrtsanta deleted the extract-server-to-lib branch September 13, 2024 12:38
@strohel strohel mentioned this pull request Dec 7, 2024
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.

3 participants