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 89659 #224

Merged
merged 5 commits into from
May 25, 2023
Merged

User Story 89659 #224

merged 5 commits into from
May 25, 2023

Conversation

TomArcherMsft
Copy link
Collaborator

@TomArcherMsft TomArcherMsft commented May 10, 2023

@TomArcherMsft TomArcherMsft changed the title User Story 89569 User Story 89659 May 10, 2023
Copy link
Member

@stemaMSFT stemaMSFT left a comment

Choose a reason for hiding this comment

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

Some minor changes then should be good @TomArcherMsft

quickstart/101-vm-cluster-linux/main.tf Outdated Show resolved Hide resolved
quickstart/101-vm-cluster-linux/readme.md Outdated Show resolved Hide resolved
@TomArcherMsft
Copy link
Collaborator Author

@stemaMSFT @lonegunmanb I've made the requested edits and ready for another review. Thanks!

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.

Hi @TomArcherMsft thanks for opening this pr! A few comments.

quickstart/101-vm-cluster-linux/main.tf Outdated Show resolved Hide resolved
quickstart/101-vm-cluster-linux/main.tf Outdated Show resolved Hide resolved
quickstart/101-vm-cluster-linux/main.tf Outdated Show resolved Hide resolved
@TomArcherMsft
Copy link
Collaborator Author

CC: @stemaMSFT

@lonegunmanb Thanks for the great review. I believe I've addressed the issues you mention. Please verify that I made the edits correctly. Thanks!

resource_group_name = azurerm_resource_group.rg.name
storage_account_type = "Standard_LRS"
create_option = "Empty"
disk_size_gb = "1023"
Copy link
Member

Choose a reason for hiding this comment

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

Why the size is not 1024 but 1023?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @TomArcherMsft, LGTM, only one question remains but the pr is good to merge.

Thanks, @lonegunmanb. I made the change.

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, LGTM, only one question remains but the pr is good to merge.

@stemaMSFT stemaMSFT merged commit 6cab78c into Azure:master May 25, 2023
@TomArcherMsft TomArcherMsft deleted the UserStory89659 branch May 30, 2023 17:41
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