Skip to content
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

Feature/prefix by description #189

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
15 changes: 14 additions & 1 deletion docs/data-sources/prefix.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,27 @@ description: |-



## Example Usage

```terraform
//Retrieve resource by cidr
resource "netbox_prefix" "cidr" {
cidr = "10.0.0.0/16"
}

//Retrieve resource by description
resource "netbox_prefix" "description" {
description = "prod-eu-west-1a"
}
```
Comment on lines +13 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

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

For a data source, a data source should be present in the example 😅


<!-- schema generated by tfplugindocs -->
## Schema

### Required
### Optional

- `cidr` (String)
- `description` (String)

### Read-Only

Expand Down
9 changes: 9 additions & 0 deletions examples/data-sources/netbox_prefix/data-source.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//Retrieve resource by cidr
resource "netbox_prefix" "cidr" {
cidr = "10.0.0.0/16"
}

//Retrieve resource by description
resource "netbox_prefix" "description" {
description = "prod-eu-west-1a"
}
66 changes: 65 additions & 1 deletion netbox/data_source_netbox_prefix.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package netbox

import (
"errors"
"fmt"
"strconv"

"github.com/fbreckle/go-netbox/netbox/client"
"github.com/fbreckle/go-netbox/netbox/client/ipam"
"github.com/fbreckle/go-netbox/netbox/models"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
)
Expand All @@ -20,9 +22,15 @@ func dataSourceNetboxPrefix() *schema.Resource {
},
"cidr": {
Type: schema.TypeString,
Required: true,
Computed: true,
Optional: true,
ValidateFunc: validation.IsCIDR,
},
"description": {
Type: schema.TypeString,
Computed: true,
Optional: true,
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

see below

}
}
Expand All @@ -31,7 +39,26 @@ func dataSourceNetboxPrefixRead(d *schema.ResourceData, m interface{}) error {
api := m.(*client.NetBoxAPI)

cidr := d.Get("cidr").(string)
description := d.Get("description").(string)

if cidr == "" && description == "" {
return errors.New("Either a prefix or a description should be given.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kind of behavior should be implemented with ExactlyOneOf in the schema definition above.

Despite that, one could also argue that filtering for both description AND cidr should be possible, if both are given. Implementation not required, though.

Choose a reason for hiding this comment

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

This kind of behavior should be implemented with ExactlyOneOf in the schema definition above.

Despite that, one could also argue that filtering for both description AND cidr should be possible, if both are given. Implementation not required, though.

This is where I doubted. I could either implement this by matching both configurations, or accept just one of the configuration options.

Which option would you pick?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, ideal would be that both options are optional and if both are given, it searches for BOTH things (with AND), whereas if only one is given, it searches for that one. Ideally, this would also apply to any search fields that are introduced later.

This might not necessarily make sense for this specific resource, but, generally speaking, searching for an intersection of multiple attributes is nice.

I would implement flexible filters (unless there are specific problems with that approach that I am not aware of). If you do not want to do that, use ExactlyOneOf.

} else if cidr == "" && description != "" {
err := dataSourceNetBoxPrefixReadByDesc(api, d, description)
if err != nil {
return err
}
} else if cidr != "" {
err := dataSourceNetBoxPrefixReadByCidr(api, d, cidr)
if err != nil {
return err
}
}

return nil
}

func dataSourceNetBoxPrefixReadByCidr(api *client.NetBoxAPI, d *schema.ResourceData, cidr string) error {
params := ipam.NewIpamPrefixesListParams()
params.Prefix = &cidr

Expand All @@ -49,8 +76,45 @@ func dataSourceNetboxPrefixRead(d *schema.ResourceData, m interface{}) error {
if *res.GetPayload().Count == int64(0) {
return errors.New("No result")
}

result := res.GetPayload().Results[0]
d.Set("id", result.ID)
d.Set("description", result.Description)
d.SetId(strconv.FormatInt(result.ID, 10))
return nil
}

func dataSourceNetBoxPrefixReadByDesc(api *client.NetBoxAPI, d *schema.ResourceData, description string) error {
params := ipam.NewIpamPrefixesListParams().WithDefaults()
params.Q = &description
fbreckle marked this conversation as resolved.
Show resolved Hide resolved

res, err := api.Ipam.IpamPrefixesList(params, nil)
if err != nil {
return err
}

if *res.GetPayload().Count == int64(0) {
return errors.New(fmt.Sprintf("No result for %s", description))
}

var hit *models.Prefix

for _, result := range res.GetPayload().Results {
if result.Description == description {
if hit != nil {
return errors.New(fmt.Sprintf("Multiple matches found for %s, can't continue.", description))
}

hit = result
}
}

if hit == nil {
return errors.New(fmt.Sprintf("No exact match found for %s", description))
}

d.Set("id", hit.ID)
d.Set("cidr", hit.Prefix)
d.SetId(strconv.FormatInt(hit.ID, 10))
return nil
}
97 changes: 96 additions & 1 deletion netbox/data_source_netbox_prefix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package netbox

import (
"fmt"
"regexp"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
)

func TestAccNetboxPrefixDataSource_basic(t *testing.T) {

testPrefix := "10.0.0.0/24"
testPrefix := "3.2.1.0/24"
resource.ParallelTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
Expand All @@ -32,3 +33,97 @@ data "netbox_prefix" "test" {
},
})
}

func TestAccNetboxPrefixDataSource_description_single(t *testing.T) {

testPrefix := "1.2.4.0/24"
testSlug := "prefix_description_single"
testDesc := testAccGetTestName(testSlug)
resource.ParallelTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: fmt.Sprintf(`
resource "netbox_prefix" "test" {
prefix = "%[1]s"
status = "active"
is_pool = true
description = "%[2]s"
}
data "netbox_prefix" "test" {
depends_on = [netbox_prefix.test]
description = "%[2]s"
}`, testPrefix, testDesc),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "id", "netbox_prefix.test", "id"),
),
ExpectNonEmptyPlan: false,
},
},
})
}

func TestAccNetboxPrefixDataSource_description_multiple_failure(t *testing.T) {

testPrefix := "1.2.3.0/24"
testPrefix2 := "1.2.64.0/24"

testSlug := "prefix_description_multiple_failure"
testDesc := testAccGetTestName(testSlug)
resource.ParallelTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: fmt.Sprintf(`
resource "netbox_prefix" "test" {
prefix = "%[1]s"
status = "active"
is_pool = true
description = "%[3]s"
}
resource "netbox_prefix" "test2" {
prefix = "%[2]s"
status = "active"
is_pool = true
description = "%[3]s"
}
data "netbox_prefix" "test" {
depends_on = [netbox_prefix.test]
description = "%[3]s"
}`, testPrefix, testPrefix2, testDesc),
ExpectError: regexp.MustCompile(fmt.Sprintf("Multiple matches found for %[1]s, can't continue.", testDesc)),
},
},
})
}

func TestAccNetboxPrefixDataSource_description_cidr(t *testing.T) {

testPrefix := "16.32.64.0/24"
testSlug := "tesst_description_cidr"
fbreckle marked this conversation as resolved.
Show resolved Hide resolved
testDesc := testAccGetTestName(testSlug)
resource.ParallelTest(t, resource.TestCase{
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: fmt.Sprintf(`
resource "netbox_prefix" "test" {
prefix = "%[1]s"
status = "active"
is_pool = true
description = "%[2]s"
}
data "netbox_prefix" "test" {
depends_on = [netbox_prefix.test]
description = "%[2]s"
}`, testPrefix, testDesc),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "id", "netbox_prefix.test", "id"),
resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "cidr", "netbox_prefix.test", "prefix"),
resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "description", "netbox_prefix.test", "description"),
),
ExpectNonEmptyPlan: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this leftover from previous testing?

},
},
})
}