Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Add tags parameter to packet_reserved_ip_block #300

Closed
wants to merge 4 commits into from

Conversation

t0mk
Copy link
Contributor

@t0mk t0mk commented Dec 3, 2020

This PR adds "tags" param to packet_reserved_ip_block.

Signed-off-by: Tomas Karasek [email protected]

@t0mk
Copy link
Contributor Author

t0mk commented Dec 3, 2020

@displague this is to partially fix #298

@displague
Copy link
Member

@t0mk the new test is among the failing tests:

--- FAIL: TestAccPacketPreCreatedIPBlock_Basic (129.10s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: 1 error occurred:
        	* Invalid address to set: []string{"tags"}
        
        
        
          on /tmp/tf-test913297341/main.tf line 16:
          (source code not available)

@t0mk
Copy link
Contributor Author

t0mk commented Dec 7, 2020

@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)
Copy link
Member

@displague displague Dec 8, 2020

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?

Copy link
Member

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.

Copy link
Contributor Author

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"}

Copy link
Member

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.

Copy link
Member

@displague displague Dec 9, 2020

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:

https://github.com/digitalocean/terraform-provider-digitalocean/blob/b3f3acbc88378f6b656735ae39e22dac59e753af/digitalocean/tags.go#L124-L135

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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

Copy link
Contributor Author

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).

Copy link
Contributor Author

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.

@displague
Copy link
Member

@t0mk I've learned that Tags can not be updated on IP Reservation blocks.
kubernetes-sigs/cloud-provider-equinix-metal#44 (comment)

@t0mk
Copy link
Contributor Author

t0mk commented Mar 23, 2021

@displague OK. Shall I move this PR to the new provider repo?

@displague
Copy link
Member

Closing this to for development in the Equinix Metal provider.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants