Skip to content
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.

Sign up for GitHub

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

Jump to bottom

azurerm_databricks_access_connector: add support for UAI #21059

Merged
merged 7 commits into from
Mar 31, 2023
Merged

azurerm_databricks_access_connector: add support for UAI #21059

merged 7 commits into from
Mar 31, 2023

Conversation

favoretti
Copy link
Collaborator 

 $ TF_ACC=1 go test -v ./internal/services/databricks -timeout=1000m -run='TestAccDatabricksAccessConnector_complete'
=== RUN   TestAccDatabricksAccessConnector_complete
=== PAUSE TestAccDatabricksAccessConnector_complete
=== CONT  TestAccDatabricksAccessConnector_complete
--- PASS: TestAccDatabricksAccessConnector_complete (152.86s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/databricks	154.071s

@favoretti favoretti force-pushed the favoretti/dbrk_access_connector_identity branch from 7b54a99 to 3b9ce8e Compare March 21, 2023 11:13
@favoretti favoretti force-pushed the favoretti/dbrk_access_connector_identity branch from 3b9ce8e to f266a5a Compare March 21, 2023 11:15
@favoretti
Copy link
Collaborator Author

image

@tombuildsstuff tombuildsstuff self-assigned this Mar 29, 2023
tombuildsstuff
tombuildsstuff suggested changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @favoretti

Thanks for this PR - I've taken a look through and this is mostly looking good to me, if we can fix up the comments then this should otherwise be good to go 👍

Thanks!

internal/services/databricks/databricks_access_connector_resource.go
@@ -42,7 +41,7 @@ func (r AccessConnectorResource) Arguments() map[string]*pluginsdk.Schema {

"resource_group_name": commonschema.ResourceGroupName(),

"identity": commonschema.SystemAssignedIdentityOptional(),
"identity": commonschema.SystemOrUserAssignedIdentityOptional(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this isn't ForceNew, we'll also want to add this to the Update method too:

			if metadata.ResourceData.HasChange("identity") {
				expandedIdentity, err := identity.ExpandLegacySystemAndUserAssignedMap(metadata.ResourceData.Get("identity").([]interface{}))
				if err != nil {
					return fmt.Errorf("expanding `identity`: %+v", err)
				}

				existing.Model.Identity = expandedIdentity
			}

(whilst I appreciate this bug was outside of this PR, since we're changing this logic anyway it seems like a good time to fix that)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

internal/services/databricks/databricks_access_connector_resource_test.go
@@ -90,10 +96,13 @@ resource "azurerm_databricks_access_connector" "test" {
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
identity {
type = "SystemAssigned"
type = "UserAssigned"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add an update test for this one, covering SystemAssigned -> UserAssigned?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

website/docs/r/databricks_access_connector.html.markdown Outdated
Comment on lines 54 to 58
* `type` - (Required) Specifies the type of Managed Service Identity that should be configured on this Access Connector. Possible values are `SystemAssigned`, `UserAssigned`, `SystemAssigned, UserAssigned` (to enable both).

* `identity_ids` - (Optional) Specifies a list of User Assigned Managed Identity IDs to be assigned to this Access Connector.

~> **NOTE:** This is required when `type` is set to `UserAssigned` or `SystemAssigned, UserAssigned`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this time the Service doesn't actually support SystemAssigned, UserAssigned even though the Swagger defines it (I've sent Azure/azure-rest-api-specs#23370 to fix that) - so we can make this:

Suggested change
* `type` - (Required) Specifies the type of Managed Service Identity that should be configured on this Access Connector. Possible values are `SystemAssigned`, `UserAssigned`, `SystemAssigned, UserAssigned` (to enable both).
* `identity_ids` - (Optional) Specifies a list of User Assigned Managed Identity IDs to be assigned to this Access Connector.
~> **NOTE:** This is required when `type` is set to `UserAssigned` or `SystemAssigned, UserAssigned`.
* `type` - (Required) Specifies the type of Managed Service Identity that should be configured on this Access Connector. Possible values are `SystemAssigned` and `UserAssigned`.
* `identity_ids` - (Optional) Specifies a list of User Assigned Managed Identity IDs to be assigned to this Access Connector.
~> **NOTE:** This is required when `type` is set to `UserAssigned`.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WodansSon haha. Thanks man. In a rush to merge this one? :) I'd get to it tomorrow otherwise.

Copy link
Collaborator

@WodansSon WodansSon Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, Yes this is an APEX ask resource so there is a release by date assigned to it... I would have normally asked if I could jump into your PR, but this PR really needed to be in tonight's release of v3.50.0 ... Sorry again for jumping in, I hope you understand... thanks. 🚀

go.mod Outdated
@@ -14,7 +14,7 @@ require (
github.com/google/go-cmp v0.5.9
github.com/google/uuid v1.1.2
github.com/hashicorp/go-azure-helpers v0.55.0
github.com/hashicorp/go-azure-sdk v0.20230317.1100159
github.com/hashicorp/go-azure-sdk v0.20230320.1165256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mind reverting/resolving the merge conflicts on this one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@tombuildsstuff tombuildsstuff mentioned this pull request Mar 30, 2023
Closed
@tombuildsstuff tombuildsstuff added this to the v3.50.0 milestone Mar 30, 2023
@github-actions github-actions bot added size/XL and removed size/L labels Mar 31, 2023
@WodansSon
Copy link
Collaborator

The go mod vendor fixed most of the conflicts, but I am not able to pull from the upstream main branch so I am not able to resolve the remaining conflicts... 😭

katbyte
katbyte approved these changes Mar 31, 2023
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🦀

@katbyte katbyte merged commit 2306f01 into hashicorp:main Mar 31, 2023
katbyte added a commit that referenced this pull request Mar 31, 2023
@katbyte
Update CHANGELOG.md #21059
1efd27a
@github-actions
Copy link

This functionality has been released in v3.50.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented May 1, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WodansSon WodansSon WodansSon left review comments

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte katbyte approved these changes

Assignees

@tombuildsstuff tombuildsstuff

Labels
dependencies documentation service/databricks size/L waiting-response
Projects
None yet
Milestone
v3.50.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@favoretti @WodansSon @tombuildsstuff @katbyte