search

databricks / terraform-provider-databricks

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

[ISSUE] Provider Update the mws_workspaces resource #351

Closed stikkireddy closed 3 years ago

stikkireddy commented 3 years ago

Hi there,

There is a field is_no_public_ip_enabled is set as omitempty and the default on server side is set to true. It is impossible for the api request to return a false via post request. https://github.com/databrickslabs/terraform-provider-databricks/blob/d8fcb5c8bcd748a77685a5a94a6e4458ed7d756d/mws/mws.go#L86-L100

The field should not be omitempty but should be defaulted to true by the terraform resource.

The pricing_tier field has been added to the workspace and that should be a new field to the workspace resource.

This should also increment the schema version.

stikkireddy commented 3 years ago

@nfx

So after going through these changes altering is_no_public_ip_enabled to true will cause issues on existing deployed MWS. we would require a state migration. From the defaulted false to true and a schema change. The reason being.

Scenario:

  1. I created a workspace before this change my statefile should say false.
  2. I make this change my statefile will be false but my default is true.
  3. Terraform will detect a change and will attempt to patch the workspace. <- This is what we want to avoid

Options will be:

  1. To ignore changes to this field in the update by documenting this and asking users to add ignore_changes = [is_no_public_ip_enabled] to existing TF code <- IMHO this is probably not a great idea as people will probably not read this docs.
  2. Schema version incremented to 1 and state is migrated by just defaulting to true. I believe setting is_no_public_ip_enabled never sent any value to the server ever because it was omitempty and the server just defaulted to what ever when the value was not sent. So by default the workspace deployed should be NPIP. There is no way of confirming because read api for that does not exist (tech debt) but this makes it such that there is minimal impact to folks
  3. Ignore this and continue to not support is_no_public_ip_enabled.
nfx commented 3 years ago
  1. Ignore this and continue to not support is_no_public_ip_enabled.

I'd prefer this option and close this issue with wontfix. is_no_public_ip_enabled is removed from documentation anyway.