databricks / terraform-provider-databricks

Databricks Terraform Provider
https://registry.terraform.io/providers/databricks/databricks/latest
Other
456 stars 393 forks source link

[FEATURE] Standardize ID fields to use strings instead of a mix of int64 and string #4231

Open eahrend opened 1 week ago

eahrend commented 1 week ago

Use-cases

Right now I'm using cdktf instead of vanilla terraform to handle large scale orchestration of our cloud environment, and we use cdktf to convert vanilla terraform into go code, this poses an issue with generation as it converts mwsworkspace.WorkspaceId() from an int64 to a float, which can't be cleanly converted to an actual usable string when we're trying to do something like string concatenation to create firewall target tags. Having it be a string would be super useful for this case.

Attempted Solutions

Tried using built in cdktf functionality to handle splitting the workspace.Id() resource since it contains the account information, but since that is a computed resource we aren't able to mutate elements of an array from a computed resource.

Proposal

It looks like this is happening in multiple places, I'm editing the title to suggest a change to convert numerical identifiers returned from the databricks API to strings rather than ints. This will have the following benefits:

  1. For cdktf users like myself, the generated code will be usable
  2. Ensure a consistent pattern for identifiers between resources
  3. In the future event where resource identifiers become alphanumeric like account IDs, it minimizes the amount of potential refactoring that's required on the provider.

References

https://github.com/databricks/terraform-provider-databricks/blob/b2c989af42c103c60ba499364eb46366d2d70f8b/mws/resource_mws_workspaces.go#L94

eahrend commented 1 week ago

I'm also seeing this for a bunch of other resources, like Principal ID for mws resource assignment expecting a number but the ID field of the group is a string.

Group ID: https://github.com/databricks/terraform-provider-databricks/blob/main/scim/scim.go#L107 MWS Resource Principal Field: https://github.com/databricks/terraform-provider-databricks/blob/b2c989af42c103c60ba499364eb46366d2d70f8b/mws/resource_mws_permission_assignment.go#L49-L50

alexott commented 6 days ago

the id field in Terraform is always string

eahrend commented 6 days ago

the id field in Terraform is always string

Right, the ID field specifically is always a string, which is fine, but things like principal_id or workspace_id are integers, when they're just identifiers for a specific resource.

When using a system like pulumi or cdktf they become difficult, if not impossible to use in string interpolation (for example, trying to do something like create a network tag suffixed with the workspace id), as those systems convert any int type field to a float, which won't accurately represent the identifier of the workspace or principal id.

My proposal is to treat these fields as strings even if the field itself is an integer

eahrend commented 6 days ago

I managed to get around this with the cdktf.Fn_ToString function, but it still seems like this should be a string of numbers rather than an actual int64