-
-
Notifications
You must be signed in to change notification settings - Fork 391
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
feat(protonvpn): Added ProtonVPN feature selection #2182
feat(protonvpn): Added ProtonVPN feature selection #2182
Conversation
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.
Nice it's looking pretty good, just a few minor comments 😉 👍
Hi, I applied (most of) your suggestions from the code review. I also added minimal unit tests (basically added to all the _test files) for all files changed in this PR. I also built my own image and started using it already. It seems to work, as far as I can tell. |
@qdm12 I've been running this code for a couple of days now myself (https://github.com/users/kvanzuijlen/packages/container/package/gluetun) and as far as I can tell it now fully works :). |
…iption tier - `validateSubscriptionTierFilters` function - `validateFeatureFilters` function - idea introduced in qdm12#2182
- `SECURE_CORE_ONLY` - `TOR_ONLY` - `P2P_ONLY`
25deac1
to
cb49a50
Compare
|
The merge-base changed after approval.
@kvanzuijlen are you aware of a difference between 'p2p' and 'port forwarding' in terms of feature for Protonvpn? I'm looking at #1103 (comment) and thinking of removing P2P_ONLY in favor of PORT_FORWARD_ONLY. Also are all p2p servers able to port forward? EDIT: Please answer on #1103 😉 Since I don't get notified for this PR anymore |
I know it's not completely done yet (missing tests, missing todo's for moving feature-related stuff to the new Features struct, docs, etc) but I wanted to open a PR to see if you agree with my chosen direction.
Closes #1582, partially #1103