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

refactor: expose public API #70

Merged
merged 3 commits into from
Mar 6, 2024
Merged

Conversation

ainghazal
Copy link
Collaborator

@ainghazal ainghazal commented Feb 21, 2024

Expose config and tunnel as a public API.

This refactor tries to expose all the useful functions to instantiate an OpenVPN tunnel (paving the way to usage from probe-cli).

Checklist

  • I have read the contribution guidelines
  • Iff you changed code related to services, or inter-service communication, make sure you update the diagrams in ARCHITECTURE.md.
  • Reference issue for this pull request:

@ainghazal
Copy link
Collaborator Author

Deferring review until we merge #68 (which is included in this branch right now).

@ainghazal ainghazal self-assigned this Mar 6, 2024
@ainghazal ainghazal requested a review from hellais March 6, 2024 00:41
Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

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

This looks good to me.

From an inspection it looks like the only thing that's really changing are the import paths for model and tun, which are moved out of the internal package.

In light of this some code is also moved around a little bit, where the most noticeable difference is the tunnel.Start method which takes the code which used to live inside of main.

The only question I have is why the logger_test and tracer_test were removed? Is it because this new layout makes testing of those packages harder?

All in all, this looks good to merge for me!

@ainghazal
Copy link
Collaborator Author

The only question I have is why the logger_test and tracer_test were removed? Is it because this new layout makes testing of those packages harder?

it was a bit of a cleanup:

  • logger_test was providing a test logger for other tests to use; this was moved to model.mocks.TestLogger
  • similarly, tracer_test provided only a no-op implementation of model.HandshakeTracer; I decided that I can pass model.DummyTracer to all tests that need to receive a tracer.

@ainghazal ainghazal merged commit 41e4e0b into ooni:main Mar 6, 2024
8 checks passed
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.

2 participants