-
Notifications
You must be signed in to change notification settings - Fork 113
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
Feature/terraform refactor for Scenarios 1 and 2 #178
Conversation
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
…dom password for vm and store in Key Vault
Terraform Plan failedPlan Error Output
Pusher: @JinLee794, Action: |
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Plan failedPlan Error Output
Pusher: @JinLee794, Action: |
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
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.
Overall, great job restructuring. Some of my comments are repeats bc I found the same thing in a couple of areas (location consistency, removing commented out code, etc.).
owner = "[email protected]" | ||
|
||
vm_aad_admin_object_id = "bda41c64-1493-4d8d-b4b5-7135159d4884" # "AppSvcLZA Azure AD SQL Admins" |
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.
Should this be hard coded here? I'm assuming this corresponds to a group created pre-deployment
run: terraform validate -no-color | ||
run: | | ||
terraform validate -no-color | ||
echo "::set-output name=stdout::$(terraform validate -no-color)" |
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.
set-output is deprecated. You'll need to do:
echo stdout=$(terraform validate -no-color) >> $GITHUB_OUTPUT
} | ||
|
||
|
||
# locals { |
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 would suggest removing any commented out code once you're committed to the module format
} | ||
variable "vm_aad_admin_object_id" { | ||
type = string | ||
description = "The Azure AD username for the VM admin account. If aad_admin_username is not specified, this value will be used." |
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.
Description needs review
|
||
variable "vm_aad_admin_username" { | ||
type = string | ||
description = "[Optional] The Azure AD username for the VM admin account. If aad_admin_object_id is not specified, this value will be used." |
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.
Description needs review
variable "location" { | ||
type = string | ||
description = "The Azure region where all resources in this example should be created" | ||
default = "westeurope" |
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.
Location consistency
error_message = "Please, choose among one of the following operating systems: Windows or Linux." | ||
} | ||
|
||
# validation { |
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.
Is this needed here?
lower("${azurerm_windows_web_app.this.name}-${azurerm_windows_web_app_slot.slot.name}.scm") | ||
] | ||
|
||
depends_on = [ |
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.
Dependency implied by ref above
variable "location" { | ||
type = string | ||
description = "The Azure region where all resources in this example should be created" | ||
default = "westeurope" |
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.
Location consistency
variable "location" { | ||
type = string | ||
description = "The Azure region where all resources in this example should be created" | ||
default = "westeurope" |
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.
Location consistency
…scenario 2 input param for location to westus2 from westus3
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Terraform Format and Style 🖌``Terraform Initialization ⚙️
|
Description
#106
Pipeline references
Type of Change
Please delete options that are not relevant.
Checklist