new resource `azurerm_stack_hci_deployment_setting` #25646

teowa · 2024-04-17T05:14:45Z

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 "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Azure Doc: https://learn.microsoft.com/en-us/azure-stack/hci/deploy/deployment-azure-resource-manager-template
REST API: https://github.com/Azure/azure-rest-api-specs/blob/c4e661cdf92c8f579574008d0cd11874cc303da0/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/deploymentSettings.json

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)

The test can only run on Windows Administrator PowerShell, and can only run based on a VM image customized to simulate Azure VM as a HCI host, since real Azure Stack HCI needs hardware to deploy.

test env:

$env:ARM_TEST_HCI_IMAGE_ID='/subscriptions/xxxx/resourceGroups/xxxx/providers/Microsoft.Compute/galleries/xxxx/images/xxxx/versions/xxxx'
$env:ARM_TEST_HCI_LOCAL_ADMIN_USER='xxxx'
$env:ARM_TEST_HCI_LOCAL_ADMIN_USER_PASSWORD='xxxx'
$env:ARM_TEST_HCI_DOMAIN_ADMIN_USER='xxxx'
$env:ARM_TEST_HCI_DOMAIN_ADMIN_USER_PASSWORD='xxxx'
$env:ARM_TEST_HCI_DEPLOYMENT_USER='xxxx'
$env:ARM_TEST_HCI_DEPLOYMENT_USER_PASSWORD=xxxx'

My local test:

>> go test -v .\internal\services\azurestackhci -run TestAccStackHCIDeploymentSetting_basic -timeout 24h
=== RUN   TestAccStackHCIDeploymentSetting_basic
=== PAUSE TestAccStackHCIDeploymentSetting_basic
=== CONT  TestAccStackHCIDeploymentSetting_basic
--- PASS: TestAccStackHCIDeploymentSetting_basic (11582.30s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/azurestackhci 18322.985s

Change Log

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

new resource azurerm_stack_hci_deployment_setting [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.

ms-zhenhua

Hi @teowa, Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

ms-zhenhua · 2024-04-26T00:53:56Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+												},
+											},
+
+											"override_adapter_property_enabled": {


please provide the default value

Suggested change

"override_adapter_property_enabled": {

"adapter_property_overriding_enabled": {

should we keep it as override_adapter_property_enabled, so it is consistent with above override_adapter_property block?

default: false added

changed to adapter_property_override_enabled

website/docs/r/stack_hci_deployment_setting.html.markdown

ms-zhenhua · 2024-04-26T03:31:51Z

website/docs/r/stack_hci_deployment_setting.html.markdown

+
+* `infrastructure_network` - (Required) One or more `infrastructure_network` blocks as defined above. Changing this forces a new Stack HCI Deployment Setting to be created.
+
+* `naming_prefix` - (Required) Specifies the naming prefix to deploy cluster. It must be 1-8 characters long and contain only letters, numbers and hyphens Changing this forces a new Stack HCI Deployment Setting to be created.


is it the name prefix of the cluster or something else?

this is not the cluster naming prefix, it is used in naming somthing produced during deployment.

changed to name_prefix

website/docs/r/stack_hci_deployment_setting.html.markdown

teowa · 2024-05-29T07:37:07Z

local test result:

teowa · 2024-06-19T05:38:11Z

katbyte · 2024-06-21T22:57:06Z

@ms-zhenhua is this pr ready for review or still being worked on and is a draft?

…m into stackhci_deployment_setting

jackofallops

Thanks @teowa - I've added a few minor comments below for error messages / documentation. I believe it's otherwise looking OK. If you can re-post the test results including these and previous changes since the PR was opened, I think we should be OK to merge 👍

jackofallops · 2024-08-07T09:10:07Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+
+			applianceClient := metadata.Client.ArcResourceBridge.AppliancesClient
+			if err := applianceClient.DeleteThenPoll(ctx, applianceId); err != nil {
+				return fmt.Errorf("deleting %s: %+v", applianceId, err)


Can we add some contextual information to the wrapped text here, since the user isn't managing this resource directly, it may be ver confusing to have this message presented for a resource they may be unaware of?

jackofallops · 2024-08-07T09:15:54Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+						}
+
+						if err := storageContainerClient.DeleteThenPoll(ctx, *storageContainerId); err != nil {
+							return err


Can we have this error wrapped, as above with context for the user to understand what is happening?

jackofallops · 2024-08-07T09:18:27Z

website/docs/r/stack_hci_deployment_setting.html.markdown

+
+# azurerm_stack_hci_deployment_setting
+
+Manages a Stack HCI Deployment Setting.


Can we also get a note added about the additional resources created on behalf of the user in their resource group to help ensure they are aware of their existence and importance, and that Terraform will attempt to remove these on deletion or recreation of this resource?

teowa · 2024-08-09T05:25:49Z

latest test results:

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

katbyte · 2024-09-13T03:13:33Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+			MinItems: 1,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"adou_path": {


adou? what does this mean? google doesn't come up with anything

please make the property name more clear if it is an acronym,

changed to active_directory_organizational_unit_path

katbyte · 2024-09-13T03:14:05Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+									Type:         pluginsdk.TypeString,
+									Required:     true,
+									ForceNew:     true,
+									ValidateFunc: validation.StringIsNotEmpty,


can we validate this? should it be a uri?

validation added, it's a domain name like core.windows.net

katbyte · 2024-09-13T03:14:29Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+									Type:         pluginsdk.TypeString,
+									Required:     true,
+									ForceNew:     true,
+									ValidateFunc: validation.StringIsNotEmpty,


This should be storage account name, validation added.

katbyte · 2024-09-13T03:14:47Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+						Type:         pluginsdk.TypeString,
+						Required:     true,
+						ForceNew:     true,
+						ValidateFunc: validation.StringIsNotEmpty,


can we validate this is a FQDN?

validation added, it's a domain name like jumpstart.local

katbyte · 2024-09-13T03:15:24Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+												MaxItems: 1,
+												Elem: &pluginsdk.Resource{
+													Schema: map[string]*pluginsdk.Schema{
+														"jumbo_packet": {


should this be an int? and validated?

It's plain string format and related to specific OEM setting. And by test, empty string should be sent if user does not specify the property. https://github.com/Azure/azure-rest-api-specs/blob/c41b76298becce0a8cfac4e682773d5d0c78dcb2/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/deploymentSettings.json#L784-L786

katbyte · 2024-09-13T03:15:40Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+												MaxItems: 1,
+												Elem: &pluginsdk.Resource{
+													Schema: map[string]*pluginsdk.Schema{
+														"bandwidth_percentage_smb": {


this is a percentage so why is it a string?

It's plain string format and related to specific OEM setting. And by test, empty string should be sent if user does not specify the property. https://github.com/Azure/azure-rest-api-specs/blob/c41b76298becce0a8cfac4e682773d5d0c78dcb2/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/deploymentSettings.json#L784-L786

katbyte · 2024-09-13T03:16:11Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+												Elem: &pluginsdk.Resource{
+													Schema: map[string]*pluginsdk.Schema{
+														"enable_iov": {
+															Type:         pluginsdk.TypeString,


this is an "enable" so true or false but is a string? shoiuld it not be a boolean ?

It's plain string format and related to specific OEM setting. And by test, empty string should be sent if user does not specify the property. https://github.com/Azure/azure-rest-api-specs/blob/c41b76298becce0a8cfac4e682773d5d0c78dcb2/specification/azurestackhci/resource-manager/Microsoft.AzureStackHCI/stable/2024-01-01/deploymentSettings.json#L784-L786

katbyte · 2024-09-13T03:19:00Z

internal/services/azurestackhci/stack_hci_deployment_setting_resource.go

+			// the resource may exist even validation error
+			metadata.SetID(id)


i'm not sure what this means, we should not be setting the ID until after the resource is created?

updated, SetID is moved after resource is created.

…m into stackhci_deployment_setting

katbyte

LGTM 🎩

github-actions bot added documentation service/azure-stack-hci size/XL labels Apr 17, 2024

teowa marked this pull request as draft April 17, 2024 05:16

github-actions bot added waiting-response dependencies and removed waiting-response labels Apr 17, 2024

new resource stack_hci_deployment_setting

8538e65

teowa force-pushed the stackhci_deployment_setting branch from cfbabcf to 8538e65 Compare April 18, 2024 10:33

teowa mentioned this pull request Apr 22, 2024

new datasource azurerm_custom_location #25693

Closed

14 tasks

ms-zhenhua suggested changes Apr 26, 2024

View reviewed changes

github-actions bot added the waiting-response label Apr 26, 2024

github-actions bot removed the waiting-response label May 11, 2024

delete customlocation & arc bridge

1f59032

teowa force-pushed the stackhci_deployment_setting branch from b3e0542 to 1f59032 Compare May 11, 2024 02:36

teowa added 3 commits May 21, 2024 17:06

delete arc bridge before custom location

c9bf9eb

terrafmt

8ed9bd2

fix import test

fdbf6d4

disable network direct in acctest

d40ccbe

github-actions bot added the waiting-response label Jun 18, 2024

rename override properties

53acd41

github-actions bot removed the waiting-response label Jun 18, 2024

disable network direct in test

f6414be

teowa marked this pull request as ready for review June 24, 2024 07:51

teowa mentioned this pull request Jun 26, 2024

new resource azurerm_stack_hci_logical_network #26473

Merged

14 tasks

change expand/flatten func name first char to lowercase

e97e1b7

teowa mentioned this pull request Jul 4, 2024

new resource azurerm_stack_hci_storage_path #26509

Merged

14 tasks

teowa added 2 commits July 4, 2024 06:20

setID before create

c51ceaf

Merge branch 'main' of github.com:hashicorp/terraform-provider-azurer…

4eea044

…m into stackhci_deployment_setting

jackofallops added the blocked label Jul 30, 2024

jackofallops self-assigned this Jul 30, 2024

teowa added 3 commits August 1, 2024 07:35

fix test exist func

6784633

Merge branch 'main' of github.com:hashicorp/terraform-provider-azurer…

44a653a

…m into stackhci_deployment_setting

remove feature toggle

2a8949c

teowa mentioned this pull request Aug 5, 2024

new resource azurerm_stack_hci_extension #26929

Open

14 tasks

jackofallops requested changes Aug 7, 2024

View reviewed changes

github-actions bot added the waiting-response label Aug 7, 2024

jackofallops removed the blocked label Aug 7, 2024

add notes on additional resources

c0fa7eb

github-actions bot removed the waiting-response label Aug 8, 2024

katbyte requested changes Sep 13, 2024

View reviewed changes

teowa added 2 commits September 14, 2024 07:14

Merge branch 'main' of github.com:hashicorp/terraform-provider-azurer…

ce1d805

…m into stackhci_deployment_setting

add more validations

80c3c3f

github-actions bot added service/extended-location waiting-response labels Sep 14, 2024

fix build issue

3dde894

github-actions bot added waiting-response and removed waiting-response labels Sep 14, 2024

katbyte approved these changes Sep 20, 2024

View reviewed changes

katbyte merged commit ee0b793 into hashicorp:main Sep 20, 2024
34 checks passed

katbyte added a commit that referenced this pull request Sep 20, 2024

Update CHANGELOG.md #25646

492c7cf

github-actions bot added this to the v4.3.0 milestone Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource `azurerm_stack_hci_deployment_setting` #25646

new resource `azurerm_stack_hci_deployment_setting` #25646

teowa commented Apr 17, 2024 •

edited

Loading

ms-zhenhua left a comment

ms-zhenhua Apr 26, 2024

teowa May 8, 2024

teowa Jun 19, 2024

ms-zhenhua Apr 26, 2024

teowa May 8, 2024

teowa Jun 24, 2024

teowa commented May 29, 2024

teowa commented Jun 19, 2024

katbyte commented Jun 21, 2024

jackofallops left a comment

jackofallops Aug 7, 2024

jackofallops Aug 7, 2024

jackofallops Aug 7, 2024

teowa commented Aug 9, 2024

katbyte Sep 13, 2024

teowa Sep 14, 2024

katbyte Sep 13, 2024

teowa Sep 14, 2024 •

edited

Loading

katbyte Sep 13, 2024

teowa Sep 14, 2024

katbyte Sep 13, 2024

teowa Sep 14, 2024

katbyte Sep 13, 2024

teowa Sep 14, 2024 •

edited

Loading

katbyte Sep 13, 2024

teowa Sep 14, 2024 •

edited

Loading

katbyte Sep 13, 2024

teowa Sep 14, 2024

katbyte Sep 13, 2024

teowa Sep 14, 2024

katbyte left a comment

	"override_adapter_property_enabled": {
	"adapter_property_overriding_enabled": {


		* `infrastructure_network` - (Required) One or more `infrastructure_network` blocks as defined above. Changing this forces a new Stack HCI Deployment Setting to be created.

		* `naming_prefix` - (Required) Specifies the naming prefix to deploy cluster. It must be 1-8 characters long and contain only letters, numbers and hyphens Changing this forces a new Stack HCI Deployment Setting to be created.


		# azurerm_stack_hci_deployment_setting

		Manages a Stack HCI Deployment Setting.

		// the resource may exist even validation error
		metadata.SetID(id)

new resource azurerm_stack_hci_deployment_setting #25646

new resource azurerm_stack_hci_deployment_setting #25646

Conversation

teowa commented Apr 17, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

ms-zhenhua 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa commented May 29, 2024

teowa commented Jun 19, 2024

katbyte commented Jun 21, 2024

jackofallops 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

teowa commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa Sep 14, 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

teowa Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teowa Sep 14, 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

katbyte left a comment

Choose a reason for hiding this comment

new resource `azurerm_stack_hci_deployment_setting` #25646

new resource `azurerm_stack_hci_deployment_setting` #25646

teowa commented Apr 17, 2024 •

edited

Loading

teowa Sep 14, 2024 •

edited

Loading

teowa Sep 14, 2024 •

edited

Loading

teowa Sep 14, 2024 •

edited

Loading