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

User Story 89540 #231

Merged
merged 7 commits into from
Jul 7, 2023
Merged

User Story 89540 #231

merged 7 commits into from
Jul 7, 2023

Conversation

TomArcherMsft
Copy link
Collaborator

Copy link
Contributor

@grayzu grayzu 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!!

Few minor comments but otherwise looks Great!

quickstart/101-vm-cluster-windows/main.tf Outdated Show resolved Hide resolved
quickstart/101-vm-cluster-windows/main.tf Outdated Show resolved Hide resolved
depends_on = [azurerm_resource_group.rg]
}

module "network" {
Copy link
Contributor

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.

Copy link
Contributor

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/)

quickstart/101-vm-cluster-windows/providers.tf Outdated Show resolved Hide resolved
quickstart/101-vm-cluster-windows/readme.md Show resolved Hide resolved
Copy link
Contributor

@grayzu grayzu left a 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" {
Copy link
Contributor

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/)

@grayzu
Copy link
Contributor

grayzu commented Jun 26, 2023

LGTM. Running tests.

@grayzu
Copy link
Contributor

grayzu commented Jun 27, 2023

tests failed, @lonegunmanb can you take a look.

@TomArcherMsft
Copy link
Collaborator Author

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

image

@TomArcherMsft TomArcherMsft temporarily deployed to acctests June 30, 2023 01:02 — with GitHub Actions Inactive
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}"
Copy link
Member

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.

@TomArcherMsft TomArcherMsft temporarily deployed to acctests July 7, 2023 01:30 — with GitHub Actions Inactive
Copy link
Member

@lonegunmanb lonegunmanb left a 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! 🚀

@lonegunmanb lonegunmanb merged commit e9538db into Azure:master Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants