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.

Sign up for GitHub

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

Jump to bottom

New resource: Dedicated Host #5417

Closed
wants to merge 9 commits into from
Closed

New resource: Dedicated Host #5417

wants to merge 9 commits into from

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jan 16, 2020

New resource dedicated host.

Note:

  1. Because of the broken delete api on dedicated host ([Compute] Dedicated Host DELETE API is broken Azure/azure-rest-api-specs#8137), we introduced a work around here.
  2. Each subscription in azure has limit quota for dedicated host, hence you might encounter quota out of limit when running acctest in parallel, in which case just limit the parallelism by passing -parallel=N into go test

katbyte
katbyte requested changes Jan 20, 2020
View reviewed changes
Copy link
Collaborator

@katbyte katbyte 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 the pr @magodo, overall this is looking pretty good! I've left some comments inline that once addressed (and tests pass) this should be good to merge

azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go Show resolved Hide resolved
azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go Show resolved Hide resolved
@katbyte katbyte added this to the v1.42.0 milestone Jan 20, 2020
@katbyte
Copy link
Collaborator

katbyte commented Jan 20, 2020

We are also seeing some failed tests:

Test Failed

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMDedicatedHost_basic
=== PAUSE TestAccDataSourceAzureRMDedicatedHost_basic
=== CONT  TestAccDataSourceAzureRMDedicatedHost_basic
--- FAIL: TestAccDataSourceAzureRMDedicatedHost_basic (65.64s)
    testing.go:630: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: Error deleting Dedicated Host Group "acctestDHG-compute-7nfwy" (Resource Group "ACCTESTRG-COMPUTE-200120174058913485"): compute.DedicatedHostGroupsClient#Delete: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="CannotDeleteResource" Message="Can not delete resource before nested resources are deleted."

@magodo
Modify per review comment
0ae152d 
Includes:

- custom parser function
- rename `sku` to `sku_name`
- multi-line code to oneline
- increase each timeout of state polling for eventual consistency from
  5sec to 10 sec, to alleviate
@magodo
Copy link
Collaborator Author

magodo commented Jan 21, 2020

@katbyte
The failure is because the service bug as commented here. I increase the polling period from 5 sec to 10 sec for each.

@ghost ghost removed the waiting-response label Jan 21, 2020
katbyte
katbyte requested changes Jan 21, 2020
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @magodo,

This is getting quite close, just a few more mostly minor comments inline to address!

azurerm/internal/services/compute/parse/dedicated_host_test.go Outdated
Comment on lines 9 to 12
Name string
Input string
ExpectedOK bool
ExpectedValue *DedicatedHostId
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this like the supplied PR?

Name     string
		Input    string
		Expected *AppServiceResourceID

we can assume nil means an error occured

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The way did in PR, by looking through the test table, it feels like user of this function can just reply on the first return value to see whether something goes wrong, and there is no guarantee about the second return value (err).

While actually we should explicitly tell user some error occurs for invalid resource ID so that user could reply on the err to see whether something goes wrong and can use the err to log or whatever.

azurerm/internal/services/compute/resource_arm_dedicated_host.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go Outdated Show resolved Hide resolved
azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go Outdated Show resolved Hide resolved
@magodo magodo requested a review from katbyte January 22, 2020 05:42
@tombuildsstuff tombuildsstuff self-assigned this Jan 22, 2020
tombuildsstuff
tombuildsstuff suggested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @magodo

Thanks for pushing those changes - I've taken a look through and left some comments inline but this is looking good to me - if we can fix up the remaining comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

azurerm/internal/services/compute/resource_arm_dedicated_host.go
Required: true,
ForceNew: true,
ValidateFunc: validateDedicatedHostGroupName(),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than passing in the Resource Group / Host Group Name here - we can instead pass in the Host Group ID and pull this out

Copy link
Contributor

Choose a reason for hiding this comment

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

on reflection can we make this dedicated_host_group_id rather than host_group_id

azurerm/internal/services/compute/resource_arm_dedicated_host.go
if utils.ResponseWasNotFound(res.Response) {
return "NotFound", "NotFound", nil
}
return nil, "", fmt.Errorf("Error issuing read request in dedicatedHostDeletedRefreshFunc: %+v", err)
Copy link
Contributor

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("Error issuing read request in dedicatedHostDeletedRefreshFunc: %+v", err)
return nil, "", fmt.Errorf("Error checking for the status of the Host Group deletion: %+v", err)

azurerm/internal/services/compute/tests/data_source_dedicated_host_test.go
{
Config: testAccDataSourceDedicatedHost_basic(data),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrSet(data.ResourceName, "id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

checking the ID is superflurious (since this'll fail without it) - can we check the specific fields here?

azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go
resource.TestCheckResourceAttr(data.ResourceName, "license_type", string(compute.DedicatedHostLicenseTypesNone)),
resource.TestCheckResourceAttr(data.ResourceName, "auto_replace_on_failure", "true"),
resource.TestCheckResourceAttr(data.ResourceName, "platform_fault_domain", "1"),
resource.TestCheckResourceAttr(data.ResourceName, "sku_name", "DSv3-Type1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of checking the value of the specific fields we can just use the import step below to do this for us

azurerm/internal/services/compute/tests/resource_arm_dedicated_host_test.go
resource.TestCheckResourceAttr(data.ResourceName, "platform_fault_domain", "1"),
resource.TestCheckResourceAttr(data.ResourceName, "sku_name", "DSv3-Type1"),
resource.TestCheckResourceAttr(data.ResourceName, "license_type", string(compute.DedicatedHostLicenseTypesWindowsServerHybrid)),
resource.TestCheckResourceAttr(data.ResourceName, "auto_replace_on_failure", "false"),
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of checking the value of the specific fields we can just use the import step below to do this for us

website/docs/r/dedicated_host.html.markdown
```hcl
resource "azurerm_resource_group" "example" {
name = "example-rg"
location = "West US"
Copy link
Contributor

Choose a reason for hiding this comment

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

we use West Europe for all examples

Suggested change
location = "West US"
location = "West Europe"

website/docs/r/dedicated_host.html.markdown

# azurerm_dedicated_host

Manage an Dedicated Host.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Manage an Dedicated Host.
Manage a Dedicated Host within a Dedicated Host Group.

website/docs/r/dedicated_host.html.markdown
layout: "azurerm"
page_title: "Azure Resource Manager: azurerm_dedicated_host"
description: |-
Manage an Dedicated Host.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Manage an Dedicated Host.
Manage a Dedicated Host within a Dedicated Host Group.

website/docs/r/dedicated_host.html.markdown

* `platform_fault_domain` - (Required) Specify the fault domain of the Dedicated Host Group in which to create the Dedicated Host. Changing this forces a new resource to be created.

* `auto_replace_on_failure` - (Optional) Specifies whether the Dedicated Host should be replaced automatically in case of a failure. The value is defaulted to `true` when not provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

to make this consistent with other resources can we make this:

Suggested change
* `auto_replace_on_failure` - (Optional) Specifies whether the Dedicated Host should be replaced automatically in case of a failure. The value is defaulted to `true` when not provided.
* `auto_replace_on_failure` - (Optional) Specifies whether the Dedicated Host should be replaced automatically in case of a failure. Defaults to `true`.

website/docs/r/dedicated_host.html.markdown

```hcl
resource "azurerm_resource_group" "example" {
name = "example-rg"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name = "example-rg"
name = "example-resources"

@tombuildsstuff
Copy link
Contributor

hey @magodo

Thanks for this PR - so that we can get this merged in order to support Dedicated Hosts within the new VM Resource, I hope you don't mind but I'm going to push a commit to resolve the pending comments here and kick off the tests 👍

Thanks!

@tombuildsstuff tombuildsstuff mentioned this pull request Jan 24, 2020
Merged
@tombuildsstuff
Copy link
Contributor

hey @magodo

Since we need to get this merged in order to proceed with the new VM Resources and we're unable to push to this branch; I hope you don't mind but I'm going to close this in favour of #5513 which contains the changes in this PR plus a commit to fix the outstanding comments.

Thanks!

@ghost
Copy link

ghost commented Jan 27, 2020

This has been released in version 1.42.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.42.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Jan 27, 2020
@ghost
Copy link

ghost commented Mar 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte Awaiting requested review from katbyte

Assignees

@tombuildsstuff tombuildsstuff

Labels
documentation service/dedicated-hosts size/XL
Projects
None yet
Milestone
v1.42.0
Development

Successfully merging this pull request may close these issues.
3 participants
@magodo @katbyte @tombuildsstuff