-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
New resource azurerm_cognitive_account_ai_services
#26008
base: main
Are you sure you want to change the base?
Conversation
|
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 @xuzhang3
I've taken a look through and left some comments inline, but there's a few questions in there - once those are resolved/clarified we can take another look through 👍
Thanks!
@@ -114,6 +118,8 @@ The `features` block supports the following: | |||
|
|||
* `cognitive_account` - (Optional) A `cognitive_account` block as defined below. | |||
|
|||
* `cognitive_account_ai_services` - (Optional) A `cognitive_account_ai_services` block as defined below. |
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 reuse the cognitive_account
block for this purpose?
* `cognitive_account_ai_services` - (Optional) A `cognitive_account_ai_services` block as defined below. |
cognitive_account_ai_services { | ||
purge_soft_delete_on_destroy = true | ||
} |
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 reuse the cognitive_account
block for this purpose?
cognitive_account_ai_services { | |
purge_soft_delete_on_destroy = true | |
} |
internal/features/defaults.go
Outdated
CognitiveAccountAIServices: CognitiveAccountAIServicesFeatures{ | ||
PurgeSoftDeleteOnDestroy: true, | ||
}, |
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.
we can reuse the CognitiveAccount block/toggle here
internal/features/user_flags.go
Outdated
AppConfiguration AppConfigurationFeatures | ||
ApplicationInsights ApplicationInsightFeatures | ||
CognitiveAccount CognitiveAccountFeatures | ||
CognitiveAccountAIServices CognitiveAccountAIServicesFeatures |
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.
CognitiveAccountAIServices CognitiveAccountAIServicesFeatures |
internal/provider/features.go
Outdated
"cognitive_account_ai_services": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
MaxItems: 1, | ||
Elem: &pluginsdk.Resource{ | ||
Schema: map[string]*pluginsdk.Schema{ | ||
"purge_soft_delete_on_destroy": { | ||
Type: pluginsdk.TypeBool, | ||
Optional: true, | ||
Default: true, | ||
}, | ||
}, | ||
}, | ||
}, |
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 reuse the cognitive_account
block for this purpose?
"cognitive_account_ai_services": { | |
Type: pluginsdk.TypeList, | |
Optional: true, | |
MaxItems: 1, | |
Elem: &pluginsdk.Resource{ | |
Schema: map[string]*pluginsdk.Schema{ | |
"purge_soft_delete_on_destroy": { | |
Type: pluginsdk.TypeBool, | |
Optional: true, | |
Default: true, | |
}, | |
}, | |
}, | |
}, |
"public_network_access_enabled": { | ||
Type: pluginsdk.TypeBool, | ||
Optional: true, | ||
Default: true, | ||
}, |
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 expose this as public_network_access
with the constant values here?
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.
rename to public_network_access
? Sorry, I didn't get the point.
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.
We're moving away from using public_network_access_enabled
, since the Azure API is gradually supporting three values for this field (Disabled
, Enabled
and SecuredByPerimeter
) - as such new resources should expose this as a string field (we'll be making this a commonschema
field in hashicorp/go-azure-helpers#238) - and existing resources will be updated in time.
As such, can we make this a string field, public_network_access
, with the constant values being the possible values - rather than a boolean?
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.
public_network_access_enabled
has been renamed to public_network_access
// also lock on the Virtual Network ID's since modifications in the networking stack are exclusive | ||
virtualNetworkNames := make([]string, 0) | ||
for _, v := range subnetIds { | ||
subnetId, err := commonids.ParseSubnetIDInsensitively(v) |
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.
These'll be validated from the config, so can be parsed directly:
subnetId, err := commonids.ParseSubnetIDInsensitively(v) | |
subnetId, err := commonids.ParseSubnetID(v) |
return err | ||
} | ||
|
||
id := cognitiveservicesaccounts.NewAccountID(subscriptionId, model.ResourceGroupName, model.Name) |
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.
we can parse the ID from metadata.ID
|
||
networkACLs, subnetIds := expandAIServicesAccountNetworkACLs(model.NetworkACLs) | ||
|
||
// also lock on the Virtual Network ID's since modifications in the networking stack are exclusive |
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.
we'd also need to lock on the virtual network ids the resource is using (e.g. from the read function) fwiw, else we'd cause errors in other places if we update two at once?
func aiServicesAccountStateRefreshFunc(ctx context.Context, client *cognitiveservicesaccounts.CognitiveServicesAccountsClient, id cognitiveservicesaccounts.AccountId) pluginsdk.StateRefreshFunc { | ||
return func() (interface{}, string, error) { | ||
res, err := client.AccountsGet(ctx, id) | ||
if err != nil { | ||
return nil, "", fmt.Errorf("polling for %s: %+v", id, err) | ||
} | ||
|
||
if res.Model != nil && res.Model.Properties != nil && res.Model.Properties.ProvisioningState != nil { | ||
return res, string(*res.Model.Properties.ProvisioningState), nil | ||
} | ||
return nil, "", fmt.Errorf("unable to read provisioning state") | ||
} | ||
} |
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.
hashicorp/go-azure-sdk
should do this for you - is this an LRO/is there a bug in the API Definitions here?
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.
this is copied from Cognitive Account and I just remove the unnecessary properties and codes
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.
Sure, but presumably when doing so you've looked to see if these can be removed? As such, what's the API actually doing here, is the Swagger wrong and this is an LRO - or is there also an API bug going on here?
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.
thanks, will remove the refresh part. There's only one API for cognitive account, keep everything the same is my first choice.
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.
@xuzhang3 with respect, you still haven’t answered my question: what’s the API actually doing here? Can you show the HTTP Request/Response of what’s being sent/coming back over the wire, so that we can understand if there’s another issue here?
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.
@tombuildsstuff the SDK can help handle the request, so I removed the state refresh and replaced by future.Poller.PollUntilDone
2. remove refresh codes 3. add lock for vnet resources
|
@tombuildsstuff all required changes has been updated |
"key_vault_key_id": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion, | ||
}, |
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.
@xuzhang3 could we add in a mhsm_key_vault_key property like we now have in storage_account resource so we can support them properly?
|
||
"identity": commonschema.SystemAssignedUserAssignedIdentityOptional(), | ||
|
||
"local_auth_enabled": { |
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.
@xuzhang3 could wemplease update this as tom asked?
Community Note
Description
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_resource
- support for thething1
property [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.