fix(server): Return nil instead of "<nil>" with IPv4/IPv6 disabled
#723
Conversation
Why:
`"<nil>"` isn't very easy to work with in terraform. e.g. there are
functions to work with null or the empty string (""), like `compact()` or
`coalesce()`, but nothing exists to handle `"<nil>"` natively, making it an
odd one and requiring specific custom filtering constructs to work with it.
`"<nil>"` is a pretty common thing to return in golang, with tens of
usages even in the main repo[1] but HCL is not golang
hcloud-go has a very nice IsUnspecified()[1] function to detect if the
API returned an unspecified IP address and a quick check says that the
hcloud cli command uses it to provide custom text output.
What:
Switch from using unconditionally the output of net.IP.String()
function, which returns `"<nil>"`[2] if the IP has length 0, to a
conditional usage of the function, setting the address to nil otherwise.
Setting the entire struct to nil apparently locally appeases unit tests
just fine and even works in some, very simple, scenarios just fine,
returning the empty string ("") instead of `"<nil>"`
This is an API breaking change. It might not be desirable to merge
and/or require some careful handling depending on how widespread the
usage of handling `"<nil>"` is. A quick search on my side, did not find
any usages[3], but I might have very well failed.
[1] https://github.com/search?q=repo%3Agolang%2Fgo%20%22%3Cnil%3E%22&type=code
[2] https://pkg.go.dev/net#IP.String
[3] https://github.com/search?q=%22%3Cnil%3E%22+language%3AHCL&type=code
Signed-off-by: Alexandros Kosiaris <akosiaris@gmail.com>
|
Thank you very much for this improvement @akosiaris! I started the CI checks, do not worry about the integrations tests failing, our API access is currently not setup well for external contributors. I ran the full test suite locally and everything worked as expected.
I would consider this buggy behaviour of the existing implementation. IMO we can merge and publish as is, if anyone stumbles upon this, they should easily find this issue by searching for and learn about it. |
apricote
changed the title
nil instead of "<nil>" with IPv4/IPv6 disablednil instead of "<nil>" with IPv4/IPv6 disabled
|
@apricote. Happy to be of help! Thanks for reviewing and merging! |
Why:
"<nil>"isn't very easy to work with in terraform. e.g. there are functions to work with null or the empty string (""), likecompact()orcoalesce(), but nothing exists to handle"<nil>"natively, making it an odd one and requiring specific custom filtering constructs to work with it."<nil>"is a pretty common thing to return in golang, with tens of usages even in the main repo[1] but HCL is not golanghcloud-go has a very nice IsUnspecified()[1] function to detect if the API returned an unspecified IP address and a quick check says that the hcloud cli command uses it to provide custom text output.
What:
Switch from using unconditionally the output of net.IP.String() function, which returns
"<nil>"[2] if the IP has length 0, to a conditional usage of the function, setting the address to nil otherwise.Setting the entire struct to nil apparently locally appeases unit tests just fine and even works in some, very simple, scenarios just fine, returning the empty string ("") instead of
"<nil>"This is an API breaking change. It might not be desirable to merge and/or require some careful handling depending on how widespread the usage of handling
"<nil>"is. A quick search on my side, did not find any usages[3], but I might have very well failed.[1] https://github.com/search?q=repo%3Agolang%2Fgo%20%22%3Cnil%3E%22&type=code
[2] https://pkg.go.dev/net#IP.String
[3] https://github.com/search?q=%22%3Cnil%3E%22+language%3AHCL&type=code