-
Notifications
You must be signed in to change notification settings - Fork 41
Add tags parameter to packet_reserved_ip_block #300
Conversation
Signed-off-by: Tomas Karasek <[email protected]>
@displague this is to partially fix #298 |
…st to set Signed-off-by: Tomas Karasek <[email protected]>
@t0mk the new test is among the failing tests:
|
Signed-off-by: Tomas Karasek <[email protected]>
@displague ah, thanks. It was because the reserved and pre-created IPs are using the same logic to extract them to TF resource data. I tried to fix it in a general way, but I'm not sure if you will like it. |
|
||
for k := range attributeMap { | ||
if d.Get(k) == nil { | ||
delete(attributeMap, k) |
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 understand how this helps. If the current TF state value is nil
, we will ignore any fetched, updated, values from the API?
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 tests passed, which makes me wonder if we are missing some worthwhile tests.
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 d.Get() will return nil if the resource doesn't have the attribute defined.
This will filter out all the map items for attributes which are not defined in the target resource, avoiding the * Invalid address to set: []string{"tags"}
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.
If the key does exist in the schema but doesn't exist in the configuration, then the default value for that type will be returned. For strings, this is "", for numbers it is 0, etc.
As long as nil
is not the default value for any type, this sounds good. I was worried that nil
would be returned for optional fields.
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.
Looking at hashicorp/terraform#5108 (comment)
I think the problem here is that the TypeSet is not willing to take a []string{"tag"}
value, I think we may have to convert the tags to (something like) map[string]string{"0": "tag"}
to store it as tags, perhaps using a conversion function.
DO seems to do this:
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.
Again, I'm not sure how https://github.com/packethost/terraform-provider-packet/pull/300/files#diff-d9548e89dc420c68f877077eab089bc0a1c4d380ece0ad32858280a8111d94caR96-R97 would be passing if that is the case.
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.
@displague in the scheme types, nil is default only for TypeInvalid (not sure what TypeInvalid is used for but I think there's no point to set it in a resource)
https://github.com/hashicorp/terraform-plugin-sdk/blob/99085370a27805c4352748aefc0c0b3698cd72d7/helper/schema/schema.go#L2096
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.
@displague I think TypeSet accepts string slice. That's why tests succeed, and I just made a dev provider build from this PR, and I can see
"tags": [
"Tag1",
"Tag2"
],
.. in the tf state (after apply and clean plan).
I can alse change the order of the tags in main.tf, and the plan is clean (because tags are a TypeSet).
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.
hashicorp/terraform#5108 (comment) is from 2016, and one of the last comments says it weas patched.
Signed-off-by: Tomas Karasek <[email protected]>
@t0mk I've learned that Tags can not be updated on IP Reservation blocks. |
@displague OK. Shall I move this PR to the new provider repo? |
Closing this to for development in the Equinix Metal provider. |
This PR adds "tags" param to packet_reserved_ip_block.
Signed-off-by: Tomas Karasek [email protected]