[Feature] Support for identity column in sql table

[hshahconsulting]

Changes

As Databricks supports identity columns, I have added support for identity columns in databricks_sql_table.
Closes [FEATURE] Identity column specification on databricks_sql_table

Tests

added unit test that asserts a correct statement has been generated when an identity column type is specified

• make test run locally
• implement integration test
• relevant change in docs/ folder

[hectorcast-db]

Hi @hshahconsulting
Thanks for this contribution. I looks quite good. I left a couple of comment.

@edwardfeng-db Can you also take a look at this PR?

[hshahconsulting]

@hectorcast-db I have made the changes according to the feedback. Anything else that needs to be done? Also, @tanmay-db said that other changes may be needed. Any follow up on that?

[tanmay-db]

Hi @hshahconsulting, thanks for creating the PR. Can you please check if this works locally for you? You can create a local binary to test: https://github.com/databricks/terraform-provider-databricks/blob/main/CONTRIBUTING.md#developing-provider

So the steps would be to:

1. Create local binary (run make in root of repository over your PR changes)
2. Override the provider using ~/.terraformrc as mentioned in the link
3. Do terraform apply as you were doing before over your HCL.

Note: Please use dev environment / test data for this (just in case there are some issues).

[hectorcast-db]

I see some issue in the list of commits. It seems that this PR is changing preexisting commits. Can you take a look at it?

[hshahconsulting]

I see some issue in the list of commits. It seems that this PR is changing preexisting commits. Can you take a look at it?

I made a mistake with a previous rebase. The branch has now been cleaned up.

[hshahconsulting]

Hi @hshahconsulting, thanks for creating the PR. Can you please check if this works locally for you? You can create a local binary to test: https://github.com/databricks/terraform-provider-databricks/blob/main/CONTRIBUTING.md#developing-provider

So the steps would be to:

1. Create local binary (run make in root of repository over your PR changes)
2. Override the provider using ~/.terraformrc as mentioned in the link
3. Do terraform apply as you were doing before over your HCL.

Note: Please use dev environment / test data for this (just in case there are some issues).

I am trying to run this locally but I cannot stop it from downloading the terraform provider from the internet. We have a version.tf with this code

terraform { required_version = ">=1.7.1" required_providers { azurerm = { source = "hashicorp/azurerm" version = "~> 3.89" } databricks = { source = "databricks/databricks" version = "~>1.50" } } backend "azurerm" { resource_group_name = "unity-tf-backend-rg" storage_account_name = "infradevseunitytfbackend" container_name = "tf-backend" key = "terraform.tfstate" } }

[hectorcast-db]

Content looks good.
I just saw that there are issues with the git history again. Can you fix that? Then we can run the automatic workflows and merge.

[hshahconsulting]

Content looks good. I just saw that there are issues with the git history again. Can you fix that? Then we can run the automatic workflows and merge.

I am not sure what the issue is this time. I created this branch from main and then pasted in the code. Which commit is bad?

[hshahconsulting]

Content looks good. I just saw that there are issues with the git history again. Can you fix that? Then we can run the automatic workflows and merge.

Please check again @hectorcast-db

[hectorcast-db]

Hi @hshahconsulting

I did run all integration tests and this changes causes some issues related to the usage of the false default:

• This test fails:
  https://github.com/databricks/terraform-provider-databricks/blob/main/internal/acceptance/sql_table_test.go#L397
  The given reason is that the diff after apply is not empty.

 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.databricks_table.this will be read during apply
  |   # (depends on a resource or a module with changes pending)
  |  <= data "databricks_table" "this" {
  |       + id   = (known after apply)
  |       + name = "sandbox########.bar"
  |     }
  | 
  |   # databricks_sql_table.mytable will be updated in-place
  |   ~ resource "databricks_sql_table" "mytable" {
  |         id                 = "sandbox########.bar"
  |         name               = "bar"
  |         # (7 unchanged attributes hidden)
  | 
  |       ~ column {
  |           + identity = "false"
  |             name     = "id"
  |             # (3 unchanged attributes hidden)
  |         }
  |     }
  | 
  | Plan: 0 to add, 1 to change, 0 to destroy.

• This change causes a diff on all existing resources, which forces customers to update all their tables. Not a great experience, and we should avoid it if possible.

Making the default an empty string should solve both issues.

[hshahconsulting]

Hi @hshahconsulting

I did run all integration tests and this changes causes some issues related to the usage of the false default:

• This test fails:
  https://github.com/databricks/terraform-provider-databricks/blob/main/internal/acceptance/sql_table_test.go#L397
  The given reason is that the diff after apply is not empty.

 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.databricks_table.this will be read during apply
  |   # (depends on a resource or a module with changes pending)
  |  <= data "databricks_table" "this" {
  |       + id   = (known after apply)
  |       + name = "sandbox########.bar"
  |     }
  | 
  |   # databricks_sql_table.mytable will be updated in-place
  |   ~ resource "databricks_sql_table" "mytable" {
  |         id                 = "sandbox########.bar"
  |         name               = "bar"
  |         # (7 unchanged attributes hidden)
  | 
  |       ~ column {
  |           + identity = "false"
  |             name     = "id"
  |             # (3 unchanged attributes hidden)
  |         }
  |     }
  | 
  | Plan: 0 to add, 1 to change, 0 to destroy.

• This change causes a diff on all existing resources, which forces customers to update all their tables. Not a great experience, and we should avoid it if possible.

Making the default an empty string should solve both issues.

Is this correct?

Identity IdentityColumn `json:"identity,omitempty"`

I have never coded in Go Lang before so please excuse my beginner level questions hehe :D

[hshahconsulting]

Please check. I have set the default to empty string.

[hshahconsulting]

@hectorcast-db Sorry for bumping but anything else needs fixing?

[hectorcast-db]

Tests just finished, and there is still one issue. However, I don't know the API well enough to recommend a solution. I have contacted the service team and I am waiting for a response. I will describe the issue in case you know how to fix it:

The new test you added is failing. Terraform always reads the server data after an apply, and then validates that there is no diff between the server status and the local status.

However, the server does not return the Identity, so there is always a diff.

We need to be able to reconstruct the "Identity" value inside the READ method based on the server response.

[hshahconsulting]

Tests just finished, and there is still one issue. However, I don't know the API well enough to recommend a solution. I have contacted the service team and I am waiting for a response. I will describe the issue in case you know how to fix it:

The new test you added is failing. Terraform always reads the server data after an apply, and then validates that there is no diff between the server status and the local status.

However, the server does not return the Identity, so there is always a diff.

We need to be able to reconstruct the "Identity" value inside the READ method based on the server response.

I see. Appreciate the help!

[hshahconsulting]

@hectorcast-db Hello Hector. Do you have any news regarding the Service Team planning to take a look at this?

[hectorcast-db]

Hi @hshahconsulting
Sorry for the delay. I still have no response. There are multiple teams involved in this, so it's taking quite a bit of time to get an answer.

[hshahconsulting]

@hectorcast-db thanks. Could you paste the logs from the latest run of the integration tests where this fails, after I set the default value for the identity to an empty string? Perhaps I could figure something out by looking at it.

[hshahconsulting]

Duplicate of #4035