-
Notifications
You must be signed in to change notification settings - Fork 135
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
adds ovh_iploadbalancing_tcp_farm_server resource #33
Conversation
dc56680
to
8647b4d
Compare
hi @goblain regarding the ovh_ip_server; |
8647b4d
to
6f7a6ef
Compare
using private IP seems to yield satisfying result, so indeed no need for this env |
6f7a6ef
to
03a808f
Compare
7eaae55
to
3a3a775
Compare
config := meta.(*Config) | ||
|
||
newBackendServer := &IpLoadbalancingTcpFarmServer{ | ||
DisplayName: getNilStringPointer(d.Get("display_name").(string)), |
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.
i don't see the use of "getNilStringPointer"
AFAIK, there's no such thing elswhere in terraform providers. what are we trying to do here ?
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.
assuming "display_name" is not set, d.Get("display_name").(string)
returns an empty string ""
, but a string none the less. gos json.Marshal
method correctly interprets it as an empty string rather then a not set string. This is why the struct uses pointers instead of string values, so they can have nil
value. getNilStringPointer then gets the string value and turns it into a pointer to string value, that in case of len(<string>) == 0
returns nil, so that json.Marshal()
instead of generating "displayName": ""
generates "displayName": null
which OVH API expects (ie. it will complain if you will have a zero value field instead of null for attributes that can not be changed after resource creation
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.
have you tried with d.Get("display_name").(*string) ?
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.
in fact I did :) it did not work. Don't think type assertion is meant to work like that at all, and even if it did, it would have no way to distinct between nil and zero value, so instead of recovering nil
, it would recover *""
but as I said, it's not doing even that.
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.
the functions I provided here do just a bit more then what you can see in pointer retrievals for aws like :
https://github.com/terraform-providers/terraform-provider-aws/blob/80e4ae53941c6fc87239f1d704e2dfe6a4dec3b6/aws/resource_aws_mq_broker.go#L185
https://github.com/aws/aws-sdk-go/blob/71a68668c287f9ad58d0c58c78e27f08eb39fa49/aws/convert_types.go#L5
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.
ok then
could you rebase the provider so we can merge this PR ? |
3a3a775
to
e12270c
Compare
e12270c
to
8c5dfb4
Compare
rebased |
once again, thanks a lot @goblain |
This commit introduces
ovh_iploadbalancing_tcp_farm_server
.