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(cordova/apple): simplify OutlineAppleLib #1660

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

fortuna
Copy link
Collaborator

@fortuna fortuna commented Jul 8, 2023

I'm getting rid of OutlineConnectivity. Note that the Go code already resolves host names (I added tests to demonstrate), so the resolution we were doing was unnecessary.

I stopped passing the terrible OutlineTunnel to some VPN functions, passing a tunnelId and configJson instead. I may rename it transportConfig or something else that reflects it's about the transport only.
I identified a place where we are inspecting the transport config that we should eventually remove.
We need to introduce a new "tunnel config" concept that contains the tunnel id, the transport config and other parameters, but that will be later.

This is part of the effort of making the platform-code agnostic to the config.

/cc @bi-kh FYI

@fortuna fortuna requested review from a team as code owners July 8, 2023 01:18
@fortuna fortuna requested a review from jyyi1 July 8, 2023 01:18
@github-actions github-actions bot added the size/M label Jul 8, 2023
@fortuna fortuna removed request for a team July 8, 2023 01:19
@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Patch coverage: 26% and no project coverage change.

Comparison is base (bccebce) 35% compared to head (2ba612e) 36%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1660   +/-   ##
======================================
  Coverage      35%     36%           
======================================
  Files          47      46    -1     
  Lines        2856    2815   -41     
  Branches      314     314           
======================================
+ Hits         1012    1018    +6     
+ Misses       1844    1797   -47     
Flag Coverage Δ
apple 14% <26%> (+1%) ⬆️
ios 15% <26%> (+1%) ⬆️
macos 14% <26%> (+1%) ⬆️
unittests 36% <26%> (+<1%) ⬆️
www 45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...eLib/Sources/OutlineTunnelSources/OutlineVpn.swift 0% <0%> (ø)
...ib/Tests/OutlineTunnelTest/OutlineTunnelTest.swift 100% <100%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fortuna fortuna changed the title Simplify OutlineAppleLib cleanup(cordova/apple): Simplify OutlineAppleLib Jul 8, 2023
@fortuna fortuna changed the title cleanup(cordova/apple): Simplify OutlineAppleLib refactor(cordova/apple): simplify OutlineAppleLib Jul 8, 2023
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

LGTM, but we might lose the error logs for isServerReachable. We should consider adding logs to the go-tun2socks.

@fortuna fortuna merged commit 6e5bfef into master Jul 10, 2023
21 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants