-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_databricks_access_connector
: add support for UAI
#21059
azurerm_databricks_access_connector
: add support for UAI
#21059
Conversation
favoretti
commented
Mar 21, 2023
7b54a99
to
3b9ce8e
Compare
3b9ce8e
to
f266a5a
Compare
There was a problem hiding this 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!
@@ -42,7 +41,7 @@ func (r AccessConnectorResource) Arguments() map[string]*pluginsdk.Schema { | |||
|
|||
"resource_group_name": commonschema.ResourceGroupName(), | |||
|
|||
"identity": commonschema.SystemAssignedIdentityOptional(), | |||
"identity": commonschema.SystemOrUserAssignedIdentityOptional(), |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
* `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`. |
There was a problem hiding this comment.
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:
* `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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦀
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! |
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. |