Add ip property to machine struct #15
Conversation
|
|
| @@ -20,6 +20,7 @@ type machine struct { | |||
| Type string `json:"-"` | |||
| Fingerprint string `json:"fingerprint"` | |||
| Hostname string `json:"hostname"` | |||
| Ip string `json:"ip"` | |||
There was a problem hiding this comment.
Thanks for the contribution. Properties should not be added to the Machine struct unless they can be populated during activation. Right now, this will always be an empty string, because it can't be set through the SDK's API.
Originally, we didn't include the IP property in the SDK because we couldn't determine a way to obtain the machine's preferred outbound IP address, in a cross-platform way, without hitting the network with an additional request (e.g. via net.Dial("udp", "keygen.sh") or something like https://ipinfo.io).
If you can find a solution for that problem, I'd be interested in accepting a PR for it, but right now, this PR essentially does nothing and it also contains no additional tests.
There was a problem hiding this comment.
Actually it does something - it makes data (which is available) in the machine.lic file available to the consumer :-\
My application is a server application and I can at least check that the application is bound to that ip/has that server url set and otherwise refuse handling requests, so it's useful to me.
I won't fight over this, as we can also just use the hostname or additional metadata.. but I don't see why you would add it to the license file and then not make it accessible through this lib
There was a problem hiding this comment.
to be clear: I'm using offline validation and checkout the machine.lic file in the web portal. It contains the ip data, I've verified. With this patch I can access it, otherwise I can't.
There was a problem hiding this comment.
Thanks for sharing more info about your use case. Your PR did not include this context in the description, so it was hard to get the full picture of what you were wanting to do here. Ideally, we'd have a unified IP attribute that is set via the SDK during activation (to prevent confusion where IP is not set, yet e.g. hostname and platform is), but I understand in your case, the machine is created via the dashboard and not the SDK.
I need to look at the failing tests (looks unrelated to the PR) and I'll consider merging.
There was a problem hiding this comment.
Yeah, that was unfortunate - I wanted to add the description, then received a phone call and totally forgot about it.
|
Superseded by #16. This PR added the property to the private struct instead of the public struct. |
|
The IP property is now available in v2.9.1. |
|
You're totally right - I copied it to the wrong struct when I used the web editor in GitHub ... overall this PR was not my proudest work 😅 Thank you very much for your effort and taking care so quickly! |
No description provided.