Feature/prefix by description #189
Conversation
|
Hi! Thank you for your pull request. Please integrate this feature in https://github.com/e-breuninger/terraform-provider-netbox/blob/master/netbox/data_source_netbox_prefix.go, instead of creating a new resource. |
|
Sorry! This was obviously not meant to be created here already. Finalized implementation - this should be looking a lot better. |
|
You can remove the draft status once you're ready |
|
Do you know why this fails in the v3.1.4 check? |
|
Not immediately. I've been re-running jobs (across multiple Netbox versions) and have been getting random (or so it feels) fails and passes. https://github.com/Kontsnor/terraform-provider-netbox/actions/runs/2470993103 Got them all to pass, but took 9 runs for all separate jobs, only restarting the failed runs. |
|
I think the issue might be that you use the same test-description in https://github.com/e-breuninger/terraform-provider-netbox/pull/189/files#diff-596d03957edacc22be4657d81c4207d648cf36388c37461eb5dbc854f1f11c8dR40 and in https://github.com/e-breuninger/terraform-provider-netbox/pull/189/files#diff-596d03957edacc22be4657d81c4207d648cf36388c37461eb5dbc854f1f11c8dR70 Since the tests run in parallel, this might lead to the error. Usually, it helps to randomize all relevant string, for example with the |
|
(tl;dr at the bottom.) Note that the test slug is prepended to the resource being created, along with the word "test" and some random stuff. This serves not only the randomization (which is why we need it here), but also allows us to identify which test created a resource when looking at it. For example, the virtual machine basic test creates lots of resources. The vm basic test's slug is Long story short: You used the slug |
|
Sorry for taking your time 😇 Updated slugs, and tests are running successful consistently now. |
| ## 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" | ||
| } | ||
| ``` |
There was a problem hiding this comment.
For a data source, a data source should be present in the example 😅
| Type: schema.TypeString, | ||
| Computed: true, | ||
| Optional: true, | ||
| }, | ||
| }, |
| description := d.Get("description").(string) | ||
|
|
||
| if cidr == "" && description == "" { | ||
| return errors.New("Either a prefix or a description should be given.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "cidr", "netbox_prefix.test", "prefix"), | ||
| resource.TestCheckResourceAttrPair("data.netbox_prefix.test", "description", "netbox_prefix.test", "description"), | ||
| ), | ||
| ExpectNonEmptyPlan: false, |
There was a problem hiding this comment.
Is this leftover from previous testing?
Add the option to retrieve a netbox prefix by description (since there is no specific name field).
A result is only returned when there is one exact match.