Implement optional support for "name" and "id" fields in data sources

[zl0ty]

No description provided.

[patoarvizu]

I'd like to get other maintainers' opinions here, but this doesn't feel right to me because it would break any existing configuration that's using name, since that field would now be computed, and id would be required. A non-breaking approach could be to have a new data source, called something like data_user_group_by_id but that also feels a little bit weird because if we have a _by_id data source, there should be a _by_name data source for consistency.

Overall this doesn't feel to me like this is necessary because if you know the id you can look up the name. I'm flexible on this, but I'm going to review this as "Request changes" anyway until we can get an agreement.

[patoarvizu]

Marking as "Request changes" at least until another maintainer chimes in

[sahaqaa]

The main idea behind this change was: initially to find ID of the resources in CloudConnexa UI user needed to open Swagger and it was too much work, so we did data sources to work by name field, as in most cases it was unique value.

After that approx 14 October 2024 we've introduced button in CloudConnexa UI near almost (if not almost) all entities - when user press on icon ID of the resources is getting copied.

So now users can get the ID of the resources easily and it kind of makes sense to now require "id" field in data sources as required.

From another side i've checked our code and found out that we are our self is heavily dependent on "name" field in two places:

data "cloudconnexa_network" "this" {
  for_each = local.networks
  name     = each.key
}

data "cloudconnexa_network" "this" { # passed into module
  for_each = var.networks
  name     = each.key
}

If this data source "cloudconnexa_network" were to change to only require ID field that would be inconvenient as we've built data structure that is dependent on:

locals {
networks = {
    azure_test = {
      default_subnet             = "xxx.xxx.xxx/xxx"
      default_subnet_description = "azure-test_full"
      extended_easy_subnets      = {}
      extended_complex_subnets   = {}
      applications = {
        "xxxxx_app_ports" = {
          route         = [{ domain = "XXXXX.local" }]
          service_types = ["HTTP", "HTTPS"]
        custom_service_types = [{ protocol = "ICMP" }] }
		}
      ipservices = {
        "azure-lab_full" = {
          route         = ["xxx.xxx.xxx/xxx"]
          service_types = ["ANY"]
        }
        "azure-prod_full" = {
          route         = ["xxx.xxx.xxx/xxx"]
          service_types = ["ANY"]
        }
        "azure-xxx-xxx" = {
          route         = ["xxx.xxx.xxx/xxx"]
          service_types = ["ANY"]
        }
      }
    }
    aws_lab = {
      default_subnet             = "xxx.xxx.xxx/xxx"
      default_subnet_description = "Labs subnet"
      extended_easy_subnets      = {}
      extended_complex_subnets   = {}
      applications = {
        "xxxxxxx" = {
          description = "xxxxx"
          route       = [{ domain = "XXXXXXXXXXX.local" }]
        custom_service_types = [{ protocol = "TCP", from_port = 8443, to_port = 8443 }] }
      }
      ipservices = {
        "AWS_full" = {
          description   = "AWS ALLOW ALL"
          route         = ["xxx.xxx.xxx/xxx]
          service_types = ["ANY"]
        }
      }
    }
}
}

"azure_test", "aws_lab" are the "keys" in data sources above ^^

From yet another side Terraform provider still in beta and some minor changes even breakable somewhat accepted/expected

[patoarvizu]

Mmhh I'm kind of rusty on the Terraform provider SDK, but is there a way to make neither field required, and have logic that would enforce that one of the two are present?

[sahaqaa]

Current status: performed testing of next resources:

+ cloudconnexa/data_source_application.go
+ cloudconnexa/data_source_network.go
+ cloudconnexa/data_source_user_group.go

"+" sign means that all good (checked all combination of id, name, only id, only name, both id and name provided, none of id and name provided)

As for "cloudconnexa/data_source_connector.go" it is harder. Unlike all the rest of entities where API enforce having unique name with connectors we can actually have next situation:

What i'm trying to say that it was broken before, broken now and as part of this PR i believe for this resource we should use only "id" field and make it required

[sahaqaa]

Working good.

Tested like this:

cloudconnexa/data_source_application.go

Specify zero fields (no "name" and no "id")
Specify both fields ("name" and "id")
Specify only "name" with correct value
Specify only "id" with correct value
Specify only "name" with non existent value
Specify only "id" with non existent value
Check when two Applications with the same “name” exists

cloudconnexa/data_source_connector.go

Specify zero fields (no "name" and no "id")
Specify both fields ("name" and "id")
Specify only "name" with correct value
Specify only "id" with correct value
Specify only "name" with non existent value
Specify only "id" with non existent value
Check when two Connectors with the same “name” exists

cloudconnexa/data_source_network.go

Specify zero fields (no "name" and no "id")
Specify both fields ("name" and "id")
Specify only "name" with correct value
Specify only "id" with correct value
Specify only "name" with non existent value
Specify only "id" with non existent value

cloudconnexa/data_source_user_group.go

Specify zero fields (no "name" and no "id")
Specify both fields ("name" and "id")
Specify only "name" with correct value
Specify only "id" with correct value
Specify only "name" with non existent value
Specify only "id" with non existent value

[sahaqaa]

@patoarvizu could you please review this PR again?

[patoarvizu]

Alright, looks good! 👍