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
Closed

Add tags parameter to packet_reserved_ip_block #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7fe2c6
Add tags parameter to packet_reserved_ip_block
t0mk Dec 3, 2020
b73f9eb
Change type of tags attr in resource_packet_reserved_ip_block from li…
t0mk Dec 4, 2020
0014127
Check attributes to set in an reserved/precreated IP resource
t0mk Dec 7, 2020
8abeccc
comment filtering of attribtes for setMap
t0mk Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/resources/reserved_ip_block.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ The following arguments are supported:
* `type` - (Optional) Either "global_ipv4" or "public_ipv4", defaults to "public_ipv4" for backward compatibility
* `facility` - (Optional) Facility where to allocate the public IP address block, makes sense only for type==public_ipv4, must be empty for type==global_ipv4
* `description` - (Optional) Arbitrary description
* `tags` - (Optiona) String list of tags

## Attributes Reference

Expand All @@ -99,6 +100,7 @@ The following attributes are exported:
* `address_family` - Address family as integer (4 or 6)
* `public` - boolean flag whether addresses from a block are public
* `global` - boolean flag whether addresses from a block are global (i.e. can be assigned in any facility)
* `tags` - string list of tags

Idempotent reference to a first "/32" address from a reserved block might look like this:

Expand Down
27 changes: 24 additions & 3 deletions packet/resource_packet_reserved_ip_block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ func resourcePacketReservedIPBlock() *schema.Resource {
Type: schema.TypeString,
Computed: true,
}
reservedBlockSchema["tags"] = &schema.Schema{
Type: schema.TypeSet,
Optional: true,
ForceNew: true,
Elem: &schema.Schema{Type: schema.TypeString},
}

return &schema.Resource{
Create: resourcePacketReservedIPBlockCreate,
Expand Down Expand Up @@ -135,6 +141,12 @@ func resourcePacketReservedIPBlockCreate(d *schema.ResourceData, meta interface{

projectID := d.Get("project_id").(string)

if tagsRaw, tagsOk := d.GetOk("tags"); tagsOk {
for _, tag := range tagsRaw.(*schema.Set).List() {
req.Tags = append(req.Tags, tag.(string))
}
}

blockAddr, _, err := client.ProjectIPs.Request(projectID, &req)
if err != nil {
return fmt.Errorf("Error reserving IP address block: %s", err)
Expand Down Expand Up @@ -190,8 +202,7 @@ func loadBlock(d *schema.ResourceData, reservedBlock *packngo.IPAddressReservati
}
quantity = 1 << uint(bits)
}

err = setMap(d, map[string]interface{}{
attributeMap := map[string]interface{}{
"address": reservedBlock.Address,
"facility": func(d *schema.ResourceData, k string) error {
if reservedBlock.Facility == nil {
Expand All @@ -205,13 +216,23 @@ func loadBlock(d *schema.ResourceData, reservedBlock *packngo.IPAddressReservati
"address_family": reservedBlock.AddressFamily,
"cidr": reservedBlock.CIDR,
"type": typ,
"tags": reservedBlock.Tags,
"public": reservedBlock.Public,
"management": reservedBlock.Management,
"manageable": reservedBlock.Manageable,
"quantity": quantity,
"project_id": path.Base(reservedBlock.Project.Href),
"cidr_notation": fmt.Sprintf("%s/%d", reservedBlock.Network, reservedBlock.CIDR),
})
}

// filter out attibutes which are not defined in target resource
for k := range attributeMap {
if d.Get(k) == nil {
delete(attributeMap, k)
Copy link
Member

@displague displague Dec 8, 2020 •
edited
Loading

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 •
edited
Loading

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.

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.

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.

}
}

err = setMap(d, attributeMap)
return err
}

Expand Down
3 changes: 3 additions & 0 deletions packet/resource_packet_reserved_ip_block_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ resource "packet_reserved_ip_block" "test" {
facility = "ewr1"
type = "public_ipv4"
quantity = 2
tags = ["Tag1", "Tag2"]
}`, name)
}

Expand Down Expand Up @@ -92,6 +93,8 @@ func TestAccPacketReservedIPBlock_Public(t *testing.T) {
"packet_reserved_ip_block.test", "public", "true"),
resource.TestCheckResourceAttr(
"packet_reserved_ip_block.test", "management", "false"),
resource.TestCheckResourceAttr(
"packet_reserved_ip_block.test", "tags.#", "2"),
),
},
},
Expand Down