-
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
azurerm_machine_learning_compute_cluster change subnet_resource_id constraint: Respect Managed Vnet when creating compute clusters fixes #25901 #26073
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.
Thanks @hmrc87. Although part of the code was just moved from what already existed I left a couple of suggestions on the error messages which would be good to update so that they're more consistent with the rest of the provider. We also need to add some more nil checks. Once that's been fixed up we can run the tests and see if this is ready to go!
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
Co-authored-by: stephybun <steph@hashicorp.com>
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 @hmrc87, I overlooked the removal of the import required error message. Could you add that back in? Should fix the test failure that I'm seeing.
internal/services/machinelearning/machine_learning_compute_cluster_resource.go
Outdated
Show resolved
Hide resolved
@stephybun I will fix all the linter and tests errors asap |
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.
Fantastic, thanks @hmrc87 LGTM 👍
This functionality has been released in v3.105.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! |
Community Note
Description
When a Managed Vnet Setup as described in the Microsoft documentation is used a compute cluster can have NO public IP and we don't need to provide a subnet_resource_id since this is managed by Microsoft.
I have tested this setup by creating a compute cluster via cli/sdk and also querying an existing compute cluster. I both cases the subnet_ressource_id can be / was empty/null.
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Change Log
azurerm_machine_learning_compute_cluster
- subnet_ressource_id is not mandatory when node_public_ip_enabled if in a managed vnet sceenario [azurerm_machine_learning_compute_cluster subnet_resource_id should be optional, even though node_public_ip_enabled is false #25901]This is a (please select all that apply):
Related Issue(s)
Fixes #25901