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.

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_machine_learning_workspace_network_outbound_rule_private_endpoint #27874

Merged
merged 11 commits into from
Dec 19, 2024

Conversation

xuzhang3
Copy link
Contributor

@xuzhang3 xuzhang3 commented Nov 4, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Add support for Machine Learning Workspace Private Endpoint Network Outbound Rule :

  • FQDN (azurerm_machine_learning_workspace_network_outbound_rule_fqdn)
  • Private Endpoint
  • Service Tag
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_onlyApprovedOutbound
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_onlyApprovedOutbound
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_internetOutbound
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_internetOutbound
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_keyVault
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_keyVault
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_workspace
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_workspace
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_redis
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_redis
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_requiresImport
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_onlyApprovedOutbound
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_redis
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_internetOutbound
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_workspace
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_keyVault
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_requiresImport (480.79s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_workspace (552.37s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_onlyApprovedOutbound (558.66s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_keyVault (561.82s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_internetOutbound (594.98s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRulePrivateEndpoint_redis (1446.73s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/machinelearning       1464.087s

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Nov 4, 2024

CI error is not relate with this PR. All tests passed

@xuzhang3 xuzhang3 changed the title [new Resource] azurerm_machine_learning_workspace_network_outbound_rule_private_endpoint [New Resource] azurerm_machine_learning_workspace_network_outbound_rule_private_endpoint Nov 4, 2024
@xuzhang3 xuzhang3 marked this pull request as ready for review November 4, 2024 08:55
@xuzhang3 xuzhang3 requested review from katbyte and a team as code owners November 4, 2024 08:55
@github-actions github-actions bot added size/XL and removed size/XS labels Nov 20, 2024
Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Hi @xuzhang3, I had a look through this and left some comments inline, once those are addressed I can take another look. Thanks!


resId, err := resourceids.ParseAzureResourceID(model.ServiceResourceId)
if err != nil {
return fmt.Errorf(" parsing resource ID: %+v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf(" parsing resource ID: %+v", err)
return err

return err
}

future, err := client.SettingsRuleDelete(ctx, *id)
Copy link
Member

Choose a reason for hiding this comment

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

could we use SettingsRuleDeleteThenPoll here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been updated to SettingsRuleDeleteThenPoll


future, err := client.SettingsRuleDelete(ctx, *id)
if err != nil {
return fmt.Errorf("deleting Machine Learning Workspace Private Endpoint Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("deleting Machine Learning Workspace Private Endpoint Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)
return fmt.Errorf("deleting %s: %+v", id, err)

if response.WasNotFound(resp.HttpResponse) {
return pointer.To(false), nil
}
return nil, fmt.Errorf("retrieving Machine Learning Workspace Outbound Rule Private Endpoint %q: %+v", state.ID, err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("retrieving Machine Learning Workspace Outbound Rule Private Endpoint %q: %+v", state.ID, err)
return nil, fmt.Errorf("retrieving %s: %+v", state.ID, err)

specify [timeouts](https://www.terraform.io/language/resources/syntax#operation-timeouts) for certain actions:

* `create` - (Defaults to 30 minutes) Used when creating the Machine Learning Workspace Network Outbound Rule Private Endpoint.
* `update` - (Defaults to 30 minutes) Used when updating the Machine Learning Workspace Network Outbound Rule Private Endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

this resource doesn't appear to have an update func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this resource cannot be updated. I follow the behavior in the portal.

Copy link
Member

Choose a reason for hiding this comment

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

could we remove the update timeout here?

@xuzhang3
Copy link
Contributor Author

this and left some comments inline, once those are addressed I can take another look. Thanks!

Thank for reviewing. updated as suggested.

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks for updating this @xuzhang3 - I left a couple more comments inline but once those are addressed I can take another look. Thanks!

Comment on lines 206 to 208
if prop.Destination.SparkEnabled != nil {
state.SparkEnabled = *prop.Destination.SparkEnabled
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if prop.Destination.SparkEnabled != nil {
state.SparkEnabled = *prop.Destination.SparkEnabled
}
state.SparkEnabled = pointer.From(prop.Destination.SparkEnabled)

Comment on lines 210 to 212
if prop.Destination.SubresourceTarget != nil {
state.SubresourceTarget = *prop.Destination.SubresourceTarget
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if prop.Destination.SubresourceTarget != nil {
state.SubresourceTarget = *prop.Destination.SubresourceTarget
}
state.SubresourceTarget = pointer.From(prop.Destination.SubresourceTarget)

Comment on lines 214 to 216
if prop.Destination.ServiceResourceId != nil {
state.ServiceResourceId = *prop.Destination.ServiceResourceId
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if prop.Destination.ServiceResourceId != nil {
state.ServiceResourceId = *prop.Destination.ServiceResourceId
}
state.ServiceResourceId = pointer.From(prop.Destination.ServiceResourceId)

specify [timeouts](https://www.terraform.io/language/resources/syntax#operation-timeouts) for certain actions:

* `create` - (Defaults to 30 minutes) Used when creating the Machine Learning Workspace Network Outbound Rule Private Endpoint.
* `update` - (Defaults to 30 minutes) Used when updating the Machine Learning Workspace Network Outbound Rule Private Endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

could we remove the update timeout here?

@xuzhang3
Copy link
Contributor Author

Thanks for updating this @xuzhang3 - I left a couple more comments inline but once those are addressed I can take another look. Thanks!

All updated per suggested

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks @xuzhang3 LGTM!

@catriona-m catriona-m merged commit e8d5187 into hashicorp:main Dec 19, 2024
34 checks passed
@github-actions github-actions bot added this to the v4.15.0 milestone Dec 19, 2024
catriona-m added a commit that referenced this pull request Dec 19, 2024
jackofallops pushed a commit that referenced this pull request Jan 10, 2025
jackofallops added a commit that referenced this pull request Jan 10, 2025
* Update CHANGELOG.md for #28233

* Update for #28215

* Update CHANGELOG.md for #28279

* Update CHANGELOG.md #28269

* Update CHANGELOG.md #27876

* Update CHANGELOG.md #28069

* Update CHANGELOG.md for #28312

* Update CHANGELOG.md for #28278

* Update CHANGELOG.md #28311

* Update CHANGELOG.md undo 28311

* Update CHANGELOG.md #27874

* Update CHANGELOG.md

* Update CHANGELOG for #28352

* Update CHANGELOG.md for #28390

* Update CHANGELOG.md for #28398

* Update CHANGELOG.md for #28425

* Update CHANGELOG.md #28427

* Update CHANGELOG.md #28280

* Update CHANGELOG.md for #28319

* Update CHANGELOG.md #24801

* Update for #28360 #28216 #27830 #28404 #27401 #27122 #27931 #28442

* Update for #28379

* Update CHANGELOG.md for #28281

* Update for #28380

* Update for #27375

* Update for #25695

* Update CHANGELOG.md #27985

* Update CHANGELOG.md - update release date manually until can be scripted

* Update CHANGELOG.md revert date change as script available

* pre-release script updates

---------

Co-authored-by: stephybun <[email protected]>
Co-authored-by: catriona-m <[email protected]>
Co-authored-by: Wyatt Fry <[email protected]>
Co-authored-by: sreallymatt <[email protected]>
Co-authored-by: Matthew Frahry <[email protected]>
Co-authored-by: kt <[email protected]>
Copy link

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 Jan 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants