search

eduvpn / eduvpn-common

Code to be shared between eduVPN clients
MIT License
5 stars 3 forks source link

V2: Should ExpiryTimes be integrated into CurrentServer #21

Closed jwijenbergh closed 6 months ago

jwijenbergh commented 1 year ago

Right now ExpiryTimes is a separate function.

Should this be integrated into the data when the current server is obtained or should this be integrated into the configuration data?

It's not really nice that this is returned in the VPN configuration data as clients can start by having a VPN already connected (e.g. Linux), they would then not be able to query this data if ExpiryTimes is removed

rozmansi commented 1 year ago

I'd vote for either moving the expiration times to the VPN configuration, or keeping a separate ExpiryTimes().

Logically, expiration times belong to VPN configuration. However, GetConfig(), CurrentServer() and ExpiryTimes() are always called together in a sequence in the current eduVPN client for Windows implementation. So, as far as I am concerned, the expiration times could also be piggybacked on server info. But that doesn't feel very natural API.

jwijenbergh commented 6 months ago

Fine as is