`data.netbox_prefix`: add custom fields and role_id

[ad8lmondy]

This PR contains two bits of work related to data.netbox_prefix.

1. Adds role_id as a field returned by data.netbox_prefix, and allows searching by role_id.
2. Return any custom_fields defined on data.netbox_prefix

Thanks!

[fbreckle]

This lacks a test for the custom fields part.

Note that custom fields are notorious for being somewhat problematic in parallel tests, because other tests find and return them unexpectedly.

[ad8lmondy]

I wasn't quite sure what to test - I didn't implement finding a netbox_prefix by specifying any custom_fields, so it's 'just' supplying additional data.

But you are right, if I change the test to be:

resource "netbox_custom_field" "test" {
  name = "test_field"
  type = "text"
  content_types = ["ipam.prefix"]
  weight        = 100
}

resource "netbox_prefix" "testv4" {
  prefix = "%[2]s"
  status = "active"
  vrf_id = netbox_vrf.test.id
  vlan_id = netbox_vlan.test.id
  site_id = netbox_site.test.id
  role_id = netbox_ipam_role.test.id
  description = "%[1]s_description_test_idv4"
  custom_fields = {
    test_field = "test value"
  }
}

resource "netbox_prefix" "testv6" {
  prefix = "%[3]s"
  status = "active"
  vrf_id = netbox_vrf.test.id
  vlan_id = netbox_vlan.test.id
  site_id = netbox_site.test.id
  description = "%[1]s_description_test_idv6"
  custom_fields = {
    test_field = "Different test value"
  }
}

The tests pass fine - but, it's not really testing anything, since there isn't a check to see if the relevant test_field value pops out of the data "netbox_prefix" blocks. However, from my own uses, it does indeed return the right data :sweat_smile: .

However, if I leave off the custom_fields definition on testv6, I do actually get an error and fail the test:

$ TEST_FUNC=TestAccNetboxPrefixDataSource_basic make testacc-specific-test
⌛ Startup acceptance tests on http://localhost:8001 with version v3.7.6
⌛ Testing function TestAccNetboxPrefixDataSource_basic
TF_ACC=1 go test -timeout 20m -v -cover netbox/*.go -run TestAccNetboxPrefixDataSource_basic
=== RUN   TestAccNetboxPrefixDataSource_basic
=== PAUSE TestAccNetboxPrefixDataSource_basic
=== CONT  TestAccNetboxPrefixDataSource_basic
    data_source_netbox_prefix_test.go:16: Step 1/1 error: After applying this test step, the plan was not empty.
        stdout:

        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
         <= read (data resources)

        Terraform will perform the following actions:

          # data.netbox_prefix.by_family will be read during apply
          # (depends on a resource or a module with changes pending)
         <= data "netbox_prefix" "by_family" {
              + description = (known after apply)
              + family      = 6
              + id          = (known after apply)
              + role_id     = (known after apply)
              + status      = (known after apply)
              + tags        = (known after apply)
            }

          # netbox_prefix.testv6 will be updated in-place
          ~ resource "netbox_prefix" "testv6" {
              ~ custom_fields = {
                  - "test_field" = null
                }
                id            = "21"
                # (10 unchanged attributes hidden)
            }

        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccNetboxPrefixDataSource_basic (2.67s)
FAIL
coverage: 7.7% of statements
FAIL    command-line-arguments  2.696s
FAIL
make: *** [GNUmakefile:23: testacc-specific-test] Error 1

But, I feel that this is a function of custom_fields being global to the content_types you pick, and perhaps how Terraform treats nulls.

For example, for any prefixes that either have no custom_fields defined, or have test_field = null, the NetBox REST API will output from GET /api/ipam/prefixes/:

...
            "custom_fields": {
                "test_field": null
            },
...

But you can't specify "test_field": null in Terraform, as (as far as I can tell) Terraform treats it as an omission, and you end up with the same result: Terraform always wants to remove the test_field field from the custom_fields map.

Given that, as mentioned, setting custom fields is a global operation for the type targeted, is it reasonable to expect people to always specify them? For example, in my use case, I set my equivalent test_field value to "" when it's not used, and all is well.

[fbreckle]

Those are some nice points and I feel that the pains of custom fields in its current state.

Just to reiterate, what I was referring to was this:

Test A adds a custom_field F for the "ipam.prefix" prefix type. Test A creates a prefix with F set, expects the answer to contain F. Test A succeeds. (so far so good)

Meanwhile: Test B creates a prefix to test if descriptions can be nulled (or anything, really) Test B fails, because it receives an unexpected F in its response, resulting in perpetual drift.

To circumvent this issue, I usually use resource.Test( (seen here) instead of resource.ParallelTest to "scope" the custom field to a single test. I am actually not sure if that is sufficient because we still have some tests randomly failing, though.

Finally, you said

since there isn't a check to see if the relevant test_field value pops out of the data "netbox_prefix" blocks.

Why not add one?

And as a final final note: consistently handling custom fields in a unified fashion is definitely something that has to be tackled eventually, its just that new things keep popping up and my time is limited.

[ad8lmondy]

Hey @fbreckle , thanks so much for your feedback, very helpful!

I've added a separate test for the custom_fields stuff, which checks if the data is returned correctly, very similar to the example you linked to.

Let me know if you'd like to see anything more, happy to defer to your judgement :)