Make databricks_entitlements forward-compatible with new entitlements
#4763
Conversation
| func setCommonUserFields(d *schema.ResourceData, user User, username string) { | ||
| d.Set("display_name", user.DisplayName) | ||
| d.Set("active", user.Active) | ||
| d.Set("external_id", user.ExternalID) | ||
| d.Set("home", fmt.Sprintf("/Users/%s", username)) |
There was a problem hiding this comment.
We need to make sure that all these fields are set in both user and SP. I recently fixed the problem in exporter that was caused by missing external_id when you import the resource
scim/entitlements.go
Outdated
| case "workspace-access": | ||
| e.WorkspaceAccess = true | ||
| default: | ||
| tflog.Info(ctx, fmt.Sprintf("Ignoring unknown entitlement: %s", c.Value)) |
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
@mgyucht can you look with my changes? |
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
alexott
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the entitlements handling system to be forward-compatible with new entitlements by preventing provider crashes when unknown entitlements are returned from the API. Instead of using hardcoded mappings and type aliases, it introduces a structured approach using embedded structs and standard schema transformation methods.
- Removes hardcoded entitlements handling that crashed on unknown values
- Introduces a new
entitlementsstruct that can be embedded in SCIM resources - Refactors User, Group, ServicePrincipal, and Entitlements resources to use the new pattern
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scim/scim.go | Removes legacy entitlements type alias and hardcoded mappings |
| scim/entitlements.go | Adds new entitlements struct with forward-compatible handling |
| scim/resource_user.go | Refactors to use embedded entitlements struct and StructToSchema pattern |
| scim/resource_service_principal.go | Refactors to use embedded entitlements struct and StructToSchema pattern |
| scim/resource_group.go | Refactors to use embedded entitlements struct and StructToSchema pattern |
| scim/resource_entitlement.go | Updates to use new entitlements handling |
| scim/data_group.go | Updates data source to use new entitlements pattern |
| scim/groups.go | Updates function signature for new entitlements type |
| scim/resource_*_test.go | Updates test cases to use []ComplexValue instead of entitlements type |
| NEXT_CHANGELOG.md | Documents the change |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Entitlements: readEntitlementsFromData(d), | ||
| ExternalID: d.Get("external_id").(string), | ||
| DisplayName: groupResource.DisplayName, | ||
| Entitlements: groupResource.toComplexValueList(), |
There was a problem hiding this comment.
The method call should be groupResource.entitlements.toComplexValueList() to access the embedded entitlements field, not called directly on groupResource.
| Entitlements: groupResource.toComplexValueList(), | |
| Entitlements: groupResource.entitlements.toComplexValueList(), |
Copilot uses AI. Check for mistakes.
| spnId := e.SpnId | ||
| noEntitlementMessage := "invalidPath No such attribute with the name : entitlements in the current resource" | ||
| entitlements := readEntitlementsFromData(d) | ||
| entitlements := e.toComplexValueList() |
There was a problem hiding this comment.
The method call should be e.entitlements.toComplexValueList() to access the embedded entitlements field, not called directly on the entitlementsResource struct.
| entitlements := e.toComplexValueList() | |
| entitlements := e.entitlements.toComplexValueList() |
Copilot uses AI. Check for mistakes.
Changes
The current implementation of
databricks_entitlementscrashes if an unexpected entitlement is returned from the API. This makes it impossible for new entitlements to be introduced and available by default. This is different from how most resources behave, where if new fields are added to resources in the API response, they are simply dropped rather than causing the provider to crash.This PR is a clean-up of the handling of entitlements in the provider, both in the
databricks_entitlementsresource and the user, group and SP resources and data sources that expose entitlements. The entitlements are defined as a singleentitlementsstructure which can be embedded in every SCIM structure. This makes it possible to use the standard StructToSchema, DataToStructPointer and StructToData methods that are ubiquitous throughout the SCIM API.I removed the
entitlementstype alias and just use[]ComplexValuein all places where that is used to minimize confusion with the newentitlementstype. The methods on that type are no longer needed because the corresponding interactions are handled by the aforementioned methods.As part of this cleanup, I have also ported the extra fields for the User, SP, Group and Entitlements resources to the struct definition, relying on StructToSchema to handle the transformation. This keeps the treatment of these APIs more uniform w.r.t. other resources on the plugin framework.
Tests
This should be a no-op change, so no new tests are needed.
The schema of the provider should not change as a result of this PR.