created the terraform infrastructure for deploy todoapp#69
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has multiple critical violations that block task completion:
-
Missing core resource: The
azurerm_linux_virtual_machineresource is not present inmodules/compute/main.tf- this is essential for the task and must be implemented. -
Checklist violations: Location is "Korea Central" instead of "uksouth" [CHECKLIST #7], VM size is "Standard_D2s_v3" instead of "Standard_B1s" [CHECKLIST #10], Public IP allocation is "Static" instead of "Dynamic" [CHECKLIST #19], and Backend container name is "task-artifacts" instead of "tfstate" [CHECKLIST #13, #27].
-
Undefined variables:
storage_account_keyandstorage_account_nameare declared but never assigned values, which will cause Terraform to fail. -
Circular dependency: The storage module receives
module.storage.storage_account_namebut generates its own name internally viarandom_stringand doesn't use the input - this creates a broken reference.
These issues prevent the infrastructure from being deployed correctly per requirements and will cause runtime failures during terraform apply.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| @@ -0,0 +1,42 @@ | |||
| # This file is maintained automatically by "terraform init". | |||
There was a problem hiding this comment.
Location should be "uksouth" as per CHECKLIST ITEM #7, not "Korea Central"
| "h1:MK83TecMdabDD+HjbxdTt3emXp8G6djLj7KvvUGstM0=", | ||
| "zh:2f81bca6a3bf3d37604bf99fdb2c77d6118520aa379ab65fd28e6b76bed399cd", | ||
| "zh:3578eb79d175af9544b0dc543124d551c0fed4c48f51773ee17e1dc62e22833a", | ||
| "zh:377dbb56caea3fa1e6a6599193b55c8594204c40c054fc2ace4f576fdfe750a2", |
There was a problem hiding this comment.
VM size should be "Standard_B1s" as per CHECKLIST ITEM #10, not "Standard_D2s_v3"
| "zh:2f81bca6a3bf3d37604bf99fdb2c77d6118520aa379ab65fd28e6b76bed399cd", | ||
| "zh:3578eb79d175af9544b0dc543124d551c0fed4c48f51773ee17e1dc62e22833a", | ||
| "zh:377dbb56caea3fa1e6a6599193b55c8594204c40c054fc2ace4f576fdfe750a2", | ||
| "zh:3d1cf01929cb213ff9a0f9753e35699bf13f60f7f0f15b38f1b216fa2cbb5f72", |
| "zh:72db82697f4e694c21defa8d0efb22f71d373676d078d71d567e8b4d9a134df7", | ||
| "zh:a0fa43cf78716ff98eccd7506b017c5c487034d9d9cce88c1accdba9314a4822", | ||
| "zh:b073f60b68b0102128815251dd895ec7f24bddec84a1b725fc0777de8a78dc7e", | ||
| "zh:b601e509eb9735756b6b7ccacc15d6333769a7bb2f8ac8c394e39b29cfc6ee55", |
There was a problem hiding this comment.
Public IP allocation_method should be "Dynamic" as per CHECKLIST ITEM #19, not "Static"
| @@ -0,0 +1,16 @@ | |||
| terraform { | |||
There was a problem hiding this comment.
Checklist Item #7 Violation: Location must be uksouth as specified in requirements, but is set to Korea Central. This will deploy resources to the wrong region.
| } | ||
|
|
||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" |
There was a problem hiding this comment.
Checklist Item #10 Violation: vm_size must be Standard_B1s as specified in requirements, but is set to Standard_D2s_v3. This changes the VM specification.
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "storagejghae7" | ||
| container_name = "task-artifacts" |
There was a problem hiding this comment.
Checklist Item #27 Violation: Backend container_name should be tfstate per requirements, but is set to task-artifacts. This mismatches the required remote state configuration.
| key = "terraform.tfstate" | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Checklist Item #19 Violation: Public IP allocation_method should be Dynamic per requirements, but is set to Static. The requirements explicitly state dynamic allocation method.
| required_providers { | ||
| azurerm = { | ||
| source = "hashicorp/azurerm" | ||
| version = "3.105.0" |
There was a problem hiding this comment.
Undefined variable: storage_account_key is declared but never assigned a value. This will cause a reference error when the module tries to use it.
| @@ -0,0 +1,16 @@ | |||
| terraform { | |||
| required_providers { | |||
| azurerm = { | |||
There was a problem hiding this comment.
Undefined variable: storage_account_name is declared but never assigned a value. This will cause a reference error when the module tries to use it.
| terraform { | ||
| required_providers { | ||
| azurerm = { | ||
| source = "hashicorp/azurerm" |
There was a problem hiding this comment.
The storage_account_key variable is passed to the storage module but the storage module has its own mechanism to get the primary access key (it outputs primary_access_key). The passed value appears to be unused or incorrectly configured.
| @@ -0,0 +1,3 @@ | |||
| .terraform/ | |||
There was a problem hiding this comment.
Location is 'Korea Central' but requirement specifies 'uksouth' (CHECKLIST ITEM #7).
| } | ||
|
|
||
| os_profile_linux_config { | ||
| disable_password_authentication = true | ||
| ssh_keys { | ||
| key_data = var.ssh_key_public | ||
| path = "/home/testadmin/.ssh/authorized_keys" |
There was a problem hiding this comment.
Critical Error: The storage module is passed module.storage.storage_account_name but the storage module declares this variable and never assigns it a value (it generates its own name internally via random_string). This creates a circular/unresolved reference that will cause Terraform to fail. The storage module doesn't need storage_account_name as input since it creates its own.
| ssh_keys { | ||
| key_data = var.ssh_key_public |
There was a problem hiding this comment.
Critical Error: var.storage_account_key is used here but never defined in terraform.tfvars. This will cause a reference error.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
The storage module receives storage_account_name and storage_account_key as inputs, but the module's main.tf never uses these variables - it generates its own storage account name. These variables are unnecessary and should be removed from the module's variables.tf.
| publisher = "Canonical" | ||
| offer = "0001-com-ubuntu-server-jammy" |
There was a problem hiding this comment.
The storage_account_key variable is declared but never assigned a value. The storage module generates its own key via primary_access_key output. Remove this unused variable.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
The storage_account_name variable is declared but never assigned a value. The storage module generates its own name. Remove this unused variable.
| } | ||
|
|
||
| variable "storage_account_name" { | ||
| type = string |
There was a problem hiding this comment.
The storage_account_name variable is passed to the compute module (line 28) but is not used anywhere in the compute module's resources. This creates unnecessary coupling.
|
|
||
| variable "storage_account_name" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The storage_account_key variable passed to compute module (line 29) is not used in any compute module resources either.
| } | ||
|
|
||
| module "network" { | ||
| source = "./modules/network" |
There was a problem hiding this comment.
Checklist Item #9 Violation: The SKU should reference the proper Ubuntu 22.04 identifier. For the Ubuntu2204 marketplace image, the SKU typically follows a format like 2204-lts rather than 22_04-lts. Verify this matches the exact marketplace image plan.
|
|
||
| module "compute" { |
There was a problem hiding this comment.
Duplicate output: network_interface_id (line 7) and nic_id (line 13) both output the exact same value. Remove one of them to avoid redundancy.
| @@ -0,0 +1,17 @@ | |||
| output "vm_name" { | |||
There was a problem hiding this comment.
The storage_account_name variable is passed to the storage module but the storage module generates its own name using random_string. This passed value is not used in modules/storage/main.tf, making the variable declaration misleading.
| variable "network_security_group_name" { | ||
| description = "" |
There was a problem hiding this comment.
Checklist Item #19 Violation: Public IP allocation_method is "Static" but the requirement explicitly states "dynamic allocation method". This must be changed to "Dynamic".
| output "subnet_id" { | ||
| value = azurerm_subnet.example.id | ||
| } | ||
| output "public_ip_id" { | ||
| value = azurerm_public_ip.example.id | ||
| } | ||
| output "public_ip_address" { | ||
| value = azurerm_public_ip.example.ip_address | ||
| } | ||
|
|
||
| output "network_security_group_id" { |
There was a problem hiding this comment.
Bad Practice: Modules should NOT contain their own terraform and provider blocks (lines 1-11). This causes provider configuration conflicts and is not the intended use of modules. Remove the terraform block and provider block - modules inherit providers from the root module.
|
|
||
| resource "azurerm_storage_container" "example" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.example.name | ||
| container_access_type = "blob" |
There was a problem hiding this comment.
CHECKLIST ITEM #19 VIOLATION: allocation_method is hardcoded as "Static" but the requirements specify it should be "Dynamic". Additionally, the variable var.public_ip_allocation_method is declared but not used - it should be referenced here instead of hardcoding.
| resource "azurerm_storage_account" "example" { | ||
| name = "storage${random_string.suffix.result}" |
There was a problem hiding this comment.
The variable public_ip_allocation_method is declared in variables.tf but never used in this file. It should be referenced in the azurerm_public_ip resource instead of the hardcoded value.
| name = var.public_ip_address_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
CHECKLIST #19 VIOLATION: The allocation_method is set to "Static" but the requirement explicitly states "dynamic allocation method". Change this to "Dynamic" to match the specification.
| source = "hashicorp/azurerm" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The public_ip_allocation_method variable is declared in variables.tf (line 7) but never used in main.tf. Either remove the variable declaration or use it in the azurerm_public_ip resource to make the allocation method configurable.
| terraform { | ||
| required_providers { | ||
| azurerm = { | ||
| source = "hashicorp/azurerm" | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| provider "azurerm" { | ||
| features {} |
There was a problem hiding this comment.
The network module has redundant terraform and provider blocks (lines 1-6, 9-11) which conflict with the main configuration's provider. Modules inherit the provider from the root module, so these blocks are unnecessary and can cause provider configuration conflicts.
| @@ -0,0 +1,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
CHECKLIST ITEM #7 VIOLATION: Location is "Korea Central" but the requirement specifies "uksouth". This will deploy resources to the wrong region.
| @@ -0,0 +1,19 @@ | |||
| variable "resource_group_name" { | |||
There was a problem hiding this comment.
CHECKLIST #7 VIOLATION: location is "Korea Central" but must be "uksouth" per requirements.
| } | ||
|
|
||
| variable "storage_account_name" { | ||
| type = string |
There was a problem hiding this comment.
CHECKLIST #10 VIOLATION: vm_size is "Standard_D2s_v3" but must be "Standard_B1s" per requirements.
|
|
||
| variable "storage_account_name" { | ||
| type = string | ||
| } | ||
|
|
||
| variable "storage_container_name" { | ||
| type = string | ||
| } | ||
| variable "storage_account_key" { | ||
| description = "Storage account access key" | ||
| type = string |
There was a problem hiding this comment.
These variables are declared but never used in modules/storage/main.tf. The storage module generates its own account name via random_string. Either remove these variable declarations or implement them properly (e.g., using a data source to retrieve existing account key).
| @@ -0,0 +1,24 @@ | |||
| output "storage_account_name" { | |||
There was a problem hiding this comment.
Checklist Item #7 Violation: Location is "Korea Central" but should be "uksouth" as specified in the requirements.
| } | ||
|
|
||
| output "storage_account_id" { | ||
| value = azurerm_storage_account.example.id |
There was a problem hiding this comment.
Checklist Item #10 Violation: vm_size is "Standard_D2s_v3" but should be "Standard_B1s" as specified in the requirements.
|
|
||
| output "storage_account_id" { | ||
| value = azurerm_storage_account.example.id | ||
| } | ||
|
|
||
| output "primary_access_key" { | ||
| value = azurerm_storage_account.example.primary_access_key | ||
| } | ||
|
|
||
| output "container_name" { | ||
| value = azurerm_storage_container.example.name |
There was a problem hiding this comment.
These variables (storage_account_name and storage_account_key) are declared but never used in modules/storage/main.tf. The storage account name is generated internally using random_string, and the access key is retrieved from the created resource. Remove these unused variable declarations.
| @@ -0,0 +1,15 @@ | |||
| location = "Korea Central" | |||
There was a problem hiding this comment.
CHECKLIST ITEM #7 VIOLATION: location is set to "Korea Central" but requirements specify "uksouth". This will deploy resources to the wrong Azure region.
| network_security_group_name = "defaultnsg" | ||
| public_ip_address_name = "linuxboxpip" | ||
| vm_name = "matebox" | ||
| vm_size = "Standard_D2s_v3" |
There was a problem hiding this comment.
CHECKLIST ITEM #10 VIOLATION: vm_size is set to "Standard_D2s_v3" but requirements specify "Standard_B1s". This changes the VM specification.
| public_ip_address_name = "linuxboxpip" | ||
| vm_name = "matebox" |
There was a problem hiding this comment.
Unused variable: storage_account_name is declared but never used in the storage module's main.tf - the module generates its own storage account name via random_string. Remove this variable.
| vm_size = "Standard_D2s_v3" | ||
| ssh_key_public = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDJLJItpBN0pq2liPeZaX8mEfFs02hO1WwpwI93YbGy6b2G5DL99O96NP7a3UR9q2O8727sWDSvjX3yusUmbnw6G++vop+Fe98y22Yf5MABhAbglHkV3db0w1ZRvegCD0yp+NtebeCK4NHHZwIA4LD3yhWkmoJ4hpeM6n+EkhiPQnLZ7ZYn7RNpWMGEjyjGOzbA2UPVfV9FvGw8O1lXQqUKovbNMN6YrJFR0bPKKcabHTwn7F3p1m5j1acjiJyxqEsRusK3JAHvRVQxQne21wwU/SBBeMbcAG4/6m9t/GKtrX8naD/OIomWVro8rnoP3GlF6Nm7YAdZTJEBF45KPdqHSroJGmjiJl0tzmc6VlVOE92DLCrnE21IVQkVzvcj9nDyQVY4NQSz6ZqGvac5F5V2yLs10Ua+LU80Mg2UPDlMEBdUwfATNOLxPtmyc8TOj049QB6Ny69ee1JeuDcuDqwlRnoDhMfqB+P7WQiGlPV8Dj2IleOtqhhCGw6mM7To0+uP6KxT/Qpu1TdfOvrvyLEhfTnP4q9FDWbVPq1aidTbzdF+eBocQkyMGsB9APQGyi15VDZZvzUMqFPalHAgcDj6RCZpgzusziVAwFHbartRRCmwwjDcrm8vt8rJAKSvl52J1yPklt0K9iLZGSS5wxMcjeFPGcNSiaxdY7dlWznmRQ==" | ||
| dns_label = "matetask" | ||
| storage_account_name = "yourstorageaccount" |
There was a problem hiding this comment.
The storage_account_name variable in terraform.tfvars is defined but not used anywhere - the storage module generates its own name. This variable can be removed.
| vm_name = "matebox" | ||
| vm_size = "Standard_D2s_v3" | ||
| ssh_key_public = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDJLJItpBN0pq2liPeZaX8mEfFs02hO1WwpwI93YbGy6b2G5DL99O96NP7a3UR9q2O8727sWDSvjX3yusUmbnw6G++vop+Fe98y22Yf5MABhAbglHkV3db0w1ZRvegCD0yp+NtebeCK4NHHZwIA4LD3yhWkmoJ4hpeM6n+EkhiPQnLZ7ZYn7RNpWMGEjyjGOzbA2UPVfV9FvGw8O1lXQqUKovbNMN6YrJFR0bPKKcabHTwn7F3p1m5j1acjiJyxqEsRusK3JAHvRVQxQne21wwU/SBBeMbcAG4/6m9t/GKtrX8naD/OIomWVro8rnoP3GlF6Nm7YAdZTJEBF45KPdqHSroJGmjiJl0tzmc6VlVOE92DLCrnE21IVQkVzvcj9nDyQVY4NQSz6ZqGvac5F5V2yLs10Ua+LU80Mg2UPDlMEBdUwfATNOLxPtmyc8TOj049QB6Ny69ee1JeuDcuDqwlRnoDhMfqB+P7WQiGlPV8Dj2IleOtqhhCGw6mM7To0+uP6KxT/Qpu1TdfOvrvyLEhfTnP4q9FDWbVPq1aidTbzdF+eBocQkyMGsB9APQGyi15VDZZvzUMqFPalHAgcDj6RCZpgzusziVAwFHbartRRCmwwjDcrm8vt8rJAKSvl52J1yPklt0K9iLZGSS5wxMcjeFPGcNSiaxdY7dlWznmRQ==" | ||
| dns_label = "matetask" |
There was a problem hiding this comment.
The dns_label variable is defined but never passed to any module or used in resources. Either remove it or use it in the network module for the public IP domain_name_label.
| } | ||
| variable "storage_account_key" { | ||
| type = string |
There was a problem hiding this comment.
Critical Error: storage_account_key is declared as a variable but never assigned a value in terraform.tfvars. This undefined variable is passed to the storage module (main.tf line 55), which will cause Terraform apply to fail with a reference error. Either remove this variable or provide a value in terraform.tfvars.
| } | ||
| variable "dns_label" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
Unused variable: dns_label is declared but never used in any module or resource. The network module generates its own DNS label using matetask${random_id.dns_suffix.dec}. Either remove this variable or pass it to the network module for use in domain_name_label.
|
|
||
| variable "storage_account_name" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
Unused variable: storage_account_name is declared but never used in main.tf. The storage module generates its own storage account name internally using random_string. Either remove this variable or implement it properly (e.g., allow custom naming).
No description provided.