Conversation
| @@ -1,5 +1,7 @@ | |||
| #![allow(deprecated)] | |||
|
|
|||
| use std::sync::Arc; | |||
| pub enum VpnType { | ||
| /// WireGuard - modern, high-performance VPN protocol. | ||
| WireGuard, | ||
| OpenVpn, |
There was a problem hiding this comment.
Missing doc comment. The WireGuard variant above has one, let's add something similar
| } | ||
|
|
||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Should also derive Copy, PartialEq, Eq to match VpnType. It's a fieldless enum, Copy is free, and without PartialEq we can't write tests
|
|
||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone)] | ||
| pub enum OpenVpnAuthType { |
There was a problem hiding this comment.
Missing doc comments on the enum and its variants
nmrs/src/api/models/vpn.rs
Outdated
|
|
||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone)] | ||
| pub struct OpenVpnCredentials { |
There was a problem hiding this comment.
So this sub-struct approach deviates from the issue spec. #290 calls for flat optional fields directly on OpenVpnConfig: username: Option<String>, password: Option<String>, etc.
The problem with required sub-structs: for AuthType::Tls, you don't need credentials at all, and for AuthType::Password, you don't need certs. But both credentials and certs are mandatory fields on OpenVpnConfig, which means callers have to construct empty/dummy values for fields that don't apply to their auth type. This defeats the purpose of OpenVpnAuthType.
Also, these sub-structs are #[non_exhaustive] but have no constructors, so they can't be instantiated outside this crate.
Let's flatten these into optional fields on OpenVpnConfig as the issue describes.
nmrs/src/api/models/vpn.rs
Outdated
| pub remote: String, | ||
| pub port: u16, | ||
| pub tcp: bool, | ||
| pub credentials: OpenVpnCredentials, |
There was a problem hiding this comment.
See comment above on OpenVpnCredentials. If you keep the sub-struct approach, these should at minimum be Option<OpenVpnCredentials> and Option<OpenVpnCerts> so that auth-type-specific fields aren't always required
nmrs/src/api/models/vpn.rs
Outdated
| /// "MyVpn", | ||
| /// "vpn.example.com", | ||
| /// 51820, | ||
| /// True, |
There was a problem hiding this comment.
rust uses lowercase (i.e. true)
Also, new_credentials, new_certs, new_auth_type, and common_fields are undefined. This example won't compile as a doc test. See WireGuardConfig::new() above in this file for an example that actually constructs its arguments inline.
The trailing blank /// lines at the end should also be removed
| pub fn new( | ||
| name: impl Into<String>, | ||
| remote: impl Into<String>, | ||
| port: impl Into<u16>, |
There was a problem hiding this comment.
I am not quite sure I understand this. nothing meaningful implements Into<u16> besides u16 itself. Same for impl Into<OpenVpnCredentials> etc., since there are no From impls for those types. Just use the concrete types.
impl Into<String> is the exception because &str -> String is a common conversion
| common: common.into(), | ||
| } | ||
| } | ||
| /// Sets the DNS servers to use when connected. |
| } | ||
| } | ||
|
|
||
| //how to satisfy the common vpn config with the one above |
There was a problem hiding this comment.
Let's clean this up in your newer patches
Dandiggas
force-pushed
the
feat/add-openvpn-config
branch
from
aba40c4 to
8c4dfd1
Compare
2 participants
No description provided.