-
Notifications
You must be signed in to change notification settings - Fork 807
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
User Story 89540 #231
User Story 89540 #231
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 for the PR!!
Few minor comments but otherwise looks Great!
depends_on = [azurerm_resource_group.rg] | ||
} | ||
|
||
module "network" { |
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.
Are these verified modules? (I assume the answer is yes) If so, let's add a comment with a link to the verified module main documentation repo.
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.
I was thinking something more like this, which refers to the verified module repo, not the registry:
Below module is a Terraform Verified Module. For more information about Verified Modules see (https://github.com/azure/terraform-azure-modules/)
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.
Please update the comments to refer to TFVM repo, not the registry.
depends_on = [azurerm_resource_group.rg] | ||
} | ||
|
||
module "network" { |
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.
I was thinking something more like this, which refers to the verified module repo, not the registry:
Below module is a Terraform Verified Module. For more information about Verified Modules see (https://github.com/azure/terraform-azure-modules/)
LGTM. Running tests. |
tests failed, @lonegunmanb can you take a look. |
@grayzu From what I can tell in the error details, it appears as though removing the resource group flag to allow deletion when other resources are present is causing problems with the testing system's ability to tear down the environment after test completion |
resource_group_name = azurerm_resource_group.rg.name | ||
vnet_subnet_id = module.network.vnet_subnets[0] | ||
is_windows_image = true | ||
vm_hostname = "vm-${random_string.windows_server_vm_hostname.result}-${count.index}" |
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.
We can add delete_os_disk_on_termination = true
here and remove prevent_deletion_if_contains_resources = false
in profider azurerm
block.
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 @TomArcherMsft for the update, LGTM! 🚀
User Story 89540