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

Category filtering for NordVPN #1806

Merged
merged 1 commit into from Mar 22, 2024
Merged

Conversation

AdamHebby
Copy link
Contributor

@AdamHebby AdamHebby commented Aug 18, 2023

Aims to fix #1643

I am not a GO developer, please read this code carefully!

image

image

After talking to NordVPN support, as long as a server supports P2P, a P2P connection will be established once it's determined there is a need for it (IE seeing P2P traffic). There's no way of knowing if it has connected to the P2P server other than contacting support with the IP address or results from curl https://ipleak.net/json/. I did ask if there was a way of starting the connection and specifying P2P from the get-go to avoid any switching but they said there was no documentation on this.

I've also seen online that this switch from standard to P2P can result in you being reconnected to a "better" server, but this could also introduce latency.

TL;DR Nord claims it "just works"

Also unsure on testing? I've added a single test case for the categories - please let me know if there's anything more I can do here.

@AdamHebby AdamHebby marked this pull request as ready for review August 19, 2023 01:07
@AdamHebby
Copy link
Contributor Author

AdamHebby commented Aug 20, 2023

@qdm12 Can I get your review on this please? Also I saw you were concerned about the potential file size increase for servers.json, it's rather minimal at .9MB increase.

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Nice work 👍 💯

I left a few comments, the most important one being:

If the group can only be legacy_standard or legacy_p2p, would it be reasonable instead to just set the PortForward server model boolean to true when encountering legacy_p2p and not have categories at all?

That of course removes a lot of the fun in this PR, but it does make sense so far to me; even if there are other categories, I'm not certain there would be a usage to filter by category except for p2p? 🤔

internal/models/filters.go Outdated Show resolved Hide resolved
internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
internal/provider/utils/filtering.go Outdated Show resolved Hide resolved
internal/provider/utils/filtering.go Outdated Show resolved Hide resolved
internal/storage/filter.go Outdated Show resolved Hide resolved
Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay re-reviewing, here are some answers on previous conversations! 👍

internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
internal/provider/nordvpn/updater/models.go Outdated Show resolved Hide resolved
@qdm12
Copy link
Owner

qdm12 commented Mar 21, 2024

I've rebased and reworked your PR, just big congrats on the code, it's quite good 💯 !

I'm just going through it renaming categories to groups since that's the terminology used in the nordvpn API.

@qdm12
Copy link
Owner

qdm12 commented Mar 21, 2024

Actually never mind, checking more the response from https://api.nordvpn.com/v2/servers?limit=10

it shows Legacy category for anything not regions, so let's keep it with categories.

- update NordVPN servers data built-in
@qdm12 qdm12 merged commit b3ceece into qdm12:master Mar 22, 2024
5 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.

Feature request: Filter NordVPN servers by group
2 participants