network_integration: add network_integration resource

[jonatas-lima]

Closes #122

[stgraber]

Hi @jonatas-lima, we typically split those kind of PRs into three commits:

• docs
• config
• tests

Any chance you can edit your PR to split your change into those three chunks?

https://github.com/lxc/terraform-provider-incus/pull/155/commits for an example of the usual structure.

Thanks!

[jonatas-lima]

Hi @stgraber! Could you review again?

[breml]

In the rest of the documentation code blocks are used to point out specific options:

Suggested change

	* `type` *Optional* - The type of the network integration. Currently, only supports **ovn** type. If empty, an OVN integration will be created.
	* `type` *Optional* - The type of the network integration. Currently, only supports `ovn` type. If empty, an OVN integration will be created.

[breml]

Imports should be grouped in 3 groups: stdlib, 3rd party, project internal packages:

Suggested change

	"github.com/lxc/incus/v6/shared/api"
	"github.com/lxc/terraform-provider-incus/internal/common"
	"github.com/lxc/incus/v6/shared/api"
	"github.com/lxc/terraform-provider-incus/internal/common"

[breml]

I think, we need an additional check here to make sure, that the user does not provide the empty string "" as name, since this causes an error on Incus side while Terraform does accept it "" != null.

Suggested change

	},
	},
	Validators: []validator.String{
	stringvalidator.LengthAtLeast(1),
	},

@maveonair, @stgraber this is maybe a general question, since I see this to be missing also for other resources on the name attribute.

[stgraber]

Hmm, yeah, empty names are usually not allowed in the API.

The one exception there being for instances where an empty name is allowed but leads to Incus generating a random instance name for you, which may by itself cause issues with Terraform.

I think it'd make sense for all name properties to be required to be at least 1 character long, then leave the more detailed validation after that to Incus.

[maveonair]

100%, we should make the name mandatory if we need it.... I have to say I've never tried using an empty string before. Great catch!

[breml]

Why is description Computed? It is provided in the Terraform config, persisted in Incus and returned by the API, so this is a regular attribute in my opinion. Do I miss something?

[stgraber]

Nope, you're correct, this isn't a field which Incus computes for you, so what's pushed by Terraform is what's stored, therefore it shouldn't be marked as computed.

[jonatas-lima]

I can't use a default value if the field is not Computed:

$ TF_ACC=1 go test -v internal/network/resource_network_integration_test.go

- Schema Using Attribute Default For Non-Computed Attribute: Attribute
        "description" must be computed when using default. This is an issue with the
        provider and should be reported to the provider developers.

[stgraber]

@maveonair any idea what's going on here?

[maveonair]

@jonatas-lima is right, if we provide a default value then we must set the attribute Computed to true.

[stgraber]

Ah, that's a bit counter-intuitive :)

[breml]

Same question as for the description.

[stgraber]

The network integration type is both required and not computed on our end.
So this indeed looks wrong, it should be false on both counts.

[jonatas-lima]

I can't use a default value if the field is not Computed:

$ TF_ACC=1 go test -v internal/network/resource_network_integration_test.go

- Schema Using Attribute Default For Non-Computed Attribute: Attribute
        "type" must be computed when using default. This is an issue with the
        provider and should be reported to the provider developers.

[jonatas-lima]

I like the idea of this field being optional because it behaves like an "enum", with "ovn" as the default value (currently the only supported network integration type). This simplifies the configuration by removing the need to always specify the type explicitly.

[stgraber]

It just feels a bit odd to have the terraform provider have it use ovn as default when at the API and CLI level there is no such default and the user is required to pick the type.

[jonatas-lima]

Ok, agree on that.

[stgraber]

thanks!