[New Resource] `azurerm_machine_learning_workspace_network_outbound_rule_service_tag` #27931

xuzhang3 · 2024-11-07T03:38:27Z

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 Service Tag Network Outbound Rule :

FQDN
Private Endpoint
Service Tag azurerm_machine_learning_workspace_network_outbound_rule_service_tag

=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport (501.71s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic (503.93s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update (633.45s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/machinelearning       636.420s

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 · 2024-11-07T06:09:35Z

katbyte

we have some test failures:

xuzhang3 · 2024-12-05T07:49:06Z

Local test passed. The API throttling not happed in my test env. Will retry running the test

= RUN   TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
=== RUN   TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== PAUSE TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport
=== CONT  TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_requiresImport (510.05s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_basic (530.12s)
--- PASS: TestAccMachineLearningWorkspaceNetworkOutboundRuleServiceTag_update (603.31s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/machinelearning       606.521s

xuzhang3 · 2024-12-05T08:59:09Z

All tests passed

stephybun · 2024-12-05T09:28:45Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+			var model machineLearningWorkspaceServiceTagOutboundRuleModel
+			if err := metadata.Decode(&model); err != nil {
+				return fmt.Errorf("decoding: %+v", err)
+			}
+
+			client := metadata.Client.MachineLearning.ManagedNetwork
+			subscriptionId := metadata.Client.Account.SubscriptionId


Ordering here is not consistent with the vast majority of the provider, please order these correctly going forward

Suggested change

var model machineLearningWorkspaceServiceTagOutboundRuleModel

if err := metadata.Decode(&model); err != nil {

return fmt.Errorf("decoding: %+v", err)

}

client := metadata.Client.MachineLearning.ManagedNetwork

subscriptionId := metadata.Client.Account.SubscriptionId

client := metadata.Client.MachineLearning.ManagedNetwork

subscriptionId := metadata.Client.Account.SubscriptionId

var model machineLearningWorkspaceServiceTagOutboundRuleModel

if err := metadata.Decode(&model); err != nil {

return fmt.Errorf("decoding: %+v", err)

}

stephybun · 2024-12-05T09:29:20Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+			var model machineLearningWorkspaceServiceTagOutboundRuleModel
+			if err := metadata.Decode(&model); err != nil {
+				return fmt.Errorf("decoding: %+v", err)
+			}
+
+			client := metadata.Client.MachineLearning.ManagedNetwork


Please fix the ordering here

stephybun · 2024-12-05T09:30:10Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+			serviceTagOutboundRule := managednetwork.ServiceTagOutboundRule{
+				Type:        managednetwork.RuleTypeServiceTag,
+				Category:    pointer.To(managednetwork.RuleCategoryUserDefined),
+				Destination: &managednetwork.ServiceTagDestination{},
+			}


Why do we need to define this, it should already exist under payload.Properties

stephybun · 2024-12-05T09:32:09Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+			model := resp.Model
+			if model == nil {
+				return fmt.Errorf("retrieving %s: model was nil", id)
+			}
+
+			state := machineLearningWorkspaceServiceTagOutboundRuleModel{
+				Name: id.OutboundRuleName,
+			}
+
+			if props := model.Properties; props != nil {
+				if prop, ok := props.(managednetwork.ServiceTagOutboundRule); ok && prop.Destination != nil {
+					if prop.Destination.ServiceTag != nil {
+						state.ServiceTag = *prop.Destination.ServiceTag
+					}
+
+					if prop.Destination.Protocol != nil {
+						state.Protocol = *prop.Destination.Protocol
+					}
+
+					if prop.Destination.PortRanges != nil {
+						state.PortRanges = *prop.Destination.PortRanges
+					}
+				}
+			}


Please review how we nil check model and properties in the read when setting values into state in the contributor guidelines and update this accordingly

stephybun · 2024-12-05T09:33:05Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+			future, err := client.SettingsRuleDelete(ctx, *id)
+			if err != nil {
+				return fmt.Errorf("deleting Machine Learning Workspace Service Tag Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)
+			}
+
+			if err = future.Poller.PollUntilDone(ctx); err != nil {
+				return fmt.Errorf("waiting for deletion of Machine Learning Workspace Service Tag Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)
+			}


Suggested change

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

if err != nil {

return fmt.Errorf("deleting Machine Learning Workspace Service Tag Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)

}

if err = future.Poller.PollUntilDone(ctx); err != nil {

return fmt.Errorf("waiting for deletion of Machine Learning Workspace Service Tag Network Outbound Rule %q (Resource Group %q, Workspace %q): %+v", id.OutboundRuleName, id.ResourceGroupName, id.WorkspaceName, err)

}

if err := client.SettingsRuleDeleteThenPoll(ctx, *id); err != nil {

return fmt.Errorf("deleting %s: %+v", id, err)

}

stephybun · 2024-12-05T09:36:20Z

website/docs/r/machine_learning_workspace_network_outbound_rule_service_tag.html.markdown

+layout: "azurerm"
+page_title: "Azure Resource Manager: azurerm_machine_learning_workspace_network_outbound_rule_service_tag"
+description: |-
+  Manages an Azure Machine Learning Workspace Service Tag Network Outbound Rule .


The resource is called network outbound rule service tag, but here we're saying manages a service tag network outbound rule. Is this a tag or a rule?

Suggested change

Manages an Azure Machine Learning Workspace Service Tag Network Outbound Rule .

Manages an Azure Machine Learning Workspace Service Tag Network Outbound Rule.

Service Tag is a type of the network rule. Others are FQNN, Private Endpoint

stephybun · 2024-12-05T09:38:18Z

...achinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource_test.go

+    resource_group {
+      prevent_deletion_if_contains_resources = false
+    }


This should never be set in acceptance tests

Suggested change

resource_group {

prevent_deletion_if_contains_resources = false

}

stephybun · 2024-12-05T09:38:31Z

...achinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource_test.go

+    resource_group {
+      prevent_deletion_if_contains_resources = false
+    }


Suggested change

resource_group {

prevent_deletion_if_contains_resources = false

}

stephybun · 2024-12-05T09:39:29Z

website/docs/r/machine_learning_workspace_network_outbound_rule_service_tag.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) Specifies the name of the Machine Learning Workspace FQDN Network Outbound Rule. Changing this forces a new resource to be created.


Now we're calling this an FQDN Network Outbound Rule here when there was no mention of it before. Is FQDN relevant here?

stephybun · 2024-12-05T09:43:02Z

website/docs/r/machine_learning_workspace_network_outbound_rule_service_tag.html.markdown

We're referring to this resource in 4 different ways

resource name: azurerm_machine_learning_workspace_network_outbound_rule_service_tag
description: azure machine learning workspace service tag network outbound rule
id attributes: Machine Learning Workspace Network Outbound Rule
imports and name attribute: Machine Learning Workspace FQDN Network Outbound Rule

Can you please update the doc to refer to the resource consistently

Docs has been updated as suggested

catriona-m

Thanks @xuzhang3 I had a look through this and spotted a few minor things that I commented inline, but once those are fixed up I can take another look. Thanks!

catriona-m · 2024-12-06T15:37:50Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+
+var _ sdk.ResourceWithUpdate = WorkspaceNetworkOutboundRuleServiceTag{}
+
+type machineLearningWorkspaceServiceTagOutboundRuleModel struct {


Suggested change

type machineLearningWorkspaceServiceTagOutboundRuleModel struct {

type MachineLearningWorkspaceServiceTagOutboundRuleModel struct {

catriona-m · 2024-12-06T15:59:54Z

...ces/machinelearning/machine_learning_workspace_network_outbound_rule_service_tag_resource.go

+						if prop.Destination.ServiceTag != nil {
+							state.ServiceTag = *prop.Destination.ServiceTag
+						}
+
+						if prop.Destination.Protocol != nil {
+							state.Protocol = *prop.Destination.Protocol
+						}
+
+						if prop.Destination.PortRanges != nil {
+							state.PortRanges = *prop.Destination.PortRanges
+						}


Suggested change

if prop.Destination.ServiceTag != nil {

state.ServiceTag = *prop.Destination.ServiceTag

}

if prop.Destination.Protocol != nil {

state.Protocol = *prop.Destination.Protocol

}

if prop.Destination.PortRanges != nil {

state.PortRanges = *prop.Destination.PortRanges

}

state.ServiceTag = pointer.From(prop.Destination.ServiceTag)

state.Protocol = pointer.From(prop.Destination.Protocol)

state.PortRanges = pointer.From(prop.Destination.PortRanges)

catriona-m · 2024-12-06T16:19:40Z

website/docs/r/machine_learning_workspace_network_outbound_rule_service_tag.html.markdown

+
+* `protocol` - (Required) Specifies the network protocol. Possible values are `*`, `TCP`, `UDP` and `ICMP`
+
+* `port_ranges` - (Required) Specifies which ports traffic will be allowed by this rule. Provide a single port, such as 80; a port range, such as 1024-655535; or a comma-separated list of single ports and/or port ranges, such as 80,1024-655535. Provide an asterisk(*) to allow traffic on any port.


Suggested change

* `port_ranges` - (Required) Specifies which ports traffic will be allowed by this rule. Provide a single port, such as 80; a port range, such as 1024-655535; or a comma-separated list of single ports and/or port ranges, such as 80,1024-655535. Provide an asterisk(*) to allow traffic on any port.

* `port_ranges` - (Required) Specifies which ports traffic will be allowed by this rule. You can specify a single port (e.g. ` 80`) , a port range (e.g. `1024-655535`) or a comma-separated list of single ports and/or port ranges(e.g. `80,1024-655535`). `*` can be used to allow traffic on any port.

xuzhang3 · 2024-12-16T08:04:00Z

Thanks @xuzhang3 I had a look through this and spotted a few minor things that I commented inline, but once those are fixed up I can take another look. Thanks!

Thanks for reviewing, updated as suggested

catriona-m

Thanks @xuzhang3 this is looking good now, however it appears there are some conflicts needing resolved before this can be merged.

add support for MLW vnet outbound rule service tag

8f9d124

github-actions bot added documentation service/machine-learning size/XL waiting-response labels Nov 7, 2024

fix lint

48ba2ef

github-actions bot removed the waiting-response label Nov 7, 2024

xuzhang3 marked this pull request as ready for review November 7, 2024 06:09

xuzhang3 requested review from katbyte and a team as code owners November 7, 2024 06:09

rcskosir added the new-resource label Nov 8, 2024

remove local val

5d9b684

katbyte requested changes Dec 4, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 4, 2024

github-actions bot removed the waiting-response label Dec 5, 2024

stephybun requested changes Dec 5, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 5, 2024

update document and refactor code

7ae4998

github-actions bot removed the waiting-response label Dec 6, 2024

catriona-m requested changes Dec 6, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 6, 2024

update documentation and nil check

04a91bc

github-actions bot removed the waiting-response label Dec 16, 2024

catriona-m requested changes Dec 19, 2024

View reviewed changes

update upstream

313f3f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Resource] `azurerm_machine_learning_workspace_network_outbound_rule_service_tag` #27931

[New Resource] `azurerm_machine_learning_workspace_network_outbound_rule_service_tag` #27931

xuzhang3 commented Nov 7, 2024

xuzhang3 commented Nov 7, 2024

katbyte left a comment

xuzhang3 commented Dec 5, 2024

xuzhang3 commented Dec 5, 2024 •

edited

Loading

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

xuzhang3 Dec 6, 2024 •

edited

Loading

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

stephybun Dec 5, 2024

xuzhang3 Dec 6, 2024

catriona-m left a comment

catriona-m Dec 6, 2024

catriona-m Dec 6, 2024

catriona-m Dec 6, 2024

xuzhang3 commented Dec 16, 2024

catriona-m left a comment

	Manages an Azure Machine Learning Workspace Service Tag Network Outbound Rule .
	Manages an Azure Machine Learning Workspace Service Tag Network Outbound Rule.

	resource_group {
	prevent_deletion_if_contains_resources = false
	}


		The following arguments are supported:

		* `name` - (Required) Specifies the name of the Machine Learning Workspace FQDN Network Outbound Rule. Changing this forces a new resource to be created.


		var _ sdk.ResourceWithUpdate = WorkspaceNetworkOutboundRuleServiceTag{}

		type machineLearningWorkspaceServiceTagOutboundRuleModel struct {


		* `protocol` - (Required) Specifies the network protocol. Possible values are `*`, `TCP`, `UDP` and `ICMP`

		* `port_ranges` - (Required) Specifies which ports traffic will be allowed by this rule. Provide a single port, such as 80; a port range, such as 1024-655535; or a comma-separated list of single ports and/or port ranges, such as 80,1024-655535. Provide an asterisk(*) to allow traffic on any port.

[New Resource] azurerm_machine_learning_workspace_network_outbound_rule_service_tag #27931

Are you sure you want to change the base?

[New Resource] azurerm_machine_learning_workspace_network_outbound_rule_service_tag #27931

Conversation

xuzhang3 commented Nov 7, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

xuzhang3 commented Nov 7, 2024

katbyte left a comment

Choose a reason for hiding this comment

xuzhang3 commented Dec 5, 2024

xuzhang3 commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhang3 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriona-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhang3 commented Dec 16, 2024

catriona-m left a comment

Choose a reason for hiding this comment

[New Resource] `azurerm_machine_learning_workspace_network_outbound_rule_service_tag` #27931

[New Resource] `azurerm_machine_learning_workspace_network_outbound_rule_service_tag` #27931

xuzhang3 commented Dec 5, 2024 •

edited

Loading

xuzhang3 Dec 6, 2024 •

edited

Loading