Skip to content

Update storage_credential docs to reference name not id #4684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

karayount
Copy link

@karayount karayount commented May 1, 2025

Changes

Updates docs/resources/storage_credential.md examples of creating databricks_grants resources that reference a storage_credential. Previously it had indicated referencing the credential by id. In my case, the name and id are not identical, and referencing credential by id caused errors, where referencing by name did not.

It appears that this package may be written with the assumption that name and id have the same value.
On line 117 of this same storage_credential.md it says
- 'id' - ID of this storage credential - same as the 'name'.
And there is a test catalog/external_location_test.go that indicates storage_credential name and id are interchangeable

func externalLocationTemplateWithOwner(comment string, owner string) string {
	return fmt.Sprintf(`
		resource "databricks_external_location" "some" {
			name            = "external-{var.STICKY_RANDOM}"
			url             = "s3://{env.TEST_BUCKET}/some{var.STICKY_RANDOM}"
			credential_name = databricks_storage_credential.external.id
			isolation_mode  = "ISOLATION_MODE_ISOLATED"
			comment         = "%s"
			owner = "%s"
		}
	`, comment, owner)
}

Tests

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • using Go SDK
  • using TF Plugin Framework

@karayount karayount requested review from a team as code owners May 1, 2025 16:19
@karayount karayount requested review from mgyucht and removed request for a team May 1, 2025 16:19
Copy link

github-actions bot commented May 1, 2025

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

  • PR number: 4684
  • Commit SHA: 405e8656489d6a3db7b10ffe66ec830fd0642ae0

Checks will be approved automatically on success.

@alexott
Copy link
Contributor

alexott commented May 7, 2025

Show the error that you get - Storage credentials and External locations have .id the same as .name: https://github.com/databricks/terraform-provider-databricks/blob/main/catalog/resource_storage_credential.go#L90

We're using .id in the example to make sure that object is created...

@karayount
Copy link
Author

Show the error that you get - Storage credentials and External locations have .id the same as .name: https://github.com/databricks/terraform-provider-databricks/blob/main/catalog/resource_storage_credential.go#L90

We're using .id in the example to make sure that object is created...

The terraform apply failed with this error, when I used storage_credential's .id. The screenshot shows the name it's searching for is a uuid which is our id, rather than the name ("databricks-csat-data-access")
Screenshot 2025-05-07 at 8 45 02 AM

The apply succeeded when I changed .id to .name in this code
Screenshot 2025-05-07 at 8 53 09 AM

@alexott
Copy link
Contributor

alexott commented May 8, 2025

Hmmm, works fine for me:

resource "databricks_storage_credential" "aott_db_access" {
  name    = "test-access-tmp"
  comment = "test"
  azure_managed_identity {
    access_connector_id = "/subscriptions/1111/resourceGroups/test-rg/providers/Microsoft.Databricks/accessConnectors/test-access"
  }
}

resource "databricks_grants" "storage_credential_aott_db_access" {
  storage_credential = databricks_storage_credential.aott_db_access.id
  grant {
    privileges = ["ALL_PRIVILEGES"]
    principal  = "[email protected]"
  }
  grant {
    privileges = ["ALL_PRIVILEGES"]
    principal  = "[email protected]"
  }
}

@alexott
Copy link
Contributor

alexott commented May 8, 2025

Can you share the debug log as per troubleshooting guide? Is storage credential created in workspace or account context?

@mgyucht
Copy link
Contributor

mgyucht commented May 8, 2025

@alexott the difference is that @karayount is using the databricks_storage_credentials data source, which exposes the UUID ID on the id field. However, for the resource, the id is indeed the same as the name. Since you need to use the name and not the ID for the databricks_grants resource, I think this is OK.

In this case, this is a bit of technical debt about the definition of the databricks_storage_credentials resource/datasource. They probably should use the same value for id but don't.

@alexott
Copy link
Contributor

alexott commented May 8, 2025

Yeah, I missed it. But our docs use resource, not data source...

@mgyucht
Copy link
Contributor

mgyucht commented May 8, 2025

Yeah, but it will work with name if you use either, as the id of the resource is set from the name today, and the API expects the name.

@alexott
Copy link
Contributor

alexott commented May 9, 2025

@karayount I'm wondering why do we need a data source in your case if you just know the name? Instead of changing the documentation to use name I would just improve the description that say that it's a name that could be used directly for existing resources (for new resources we need to use .id to make sure that resource is created, before granting an access).

@karayount
Copy link
Author

@karayount I'm wondering why do we need a data source in your case if you just know the name? Instead of changing the documentation to use name I would just improve the description that say that it's a name that could be used directly for existing resources (for new resources we need to use .id to make sure that resource is created, before granting an access).

Ah. I will close this. I missed that the docs were for resource and I was using data, and I found the error surprising. Might be nice to add a data example to the docs too, if folks agree.

@karayount karayount closed this Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants