Solution#73
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several explicit checklist requirements are not yet satisfied in ways that are considered critical for this task. The Azure backend is configured with resource_group_name = "tfstate-rg" and a custom storage_account_name instead of the mandated resource_group_name = "mate-azure-task-12" and storage_account_name = "yourstorageaccount" with container tfstate and key terraform.tfstate (see backend.tf and related references) — this violates [CHECKLIST ITEM #8, #9, #42] and directly affects remote state. In modules/network/main.tf, the public IP uses allocation_method = "Static", but the task requires dynamic allocation ("Dynamic"), so this must be corrected to meet [CHECKLIST ITEM #12, #34]. In variables.tf (and repeated in module variable files) the location variable defaults to "denmarkeast" instead of the required "uksouth", which breaks [CHECKLIST ITEM #16] and causes all resources to be deployed in the wrong region. Across root and compute module variables you use ssh_public_key with a real key, while the checklist requires a variable named ssh_key_public with value "your-public-key-content" and an SSH key named linuxboxsshkey [CHECKLIST ITEM #15, #26]; you should rename the variable and align its value and key naming to match the specification. Functionally your modular structure, VM creation, and CustomScript extension wiring look good, so once you fix the backend settings, location default, public IP allocation method, and SSH key variable/name/value to match the checklist, your solution will be very close to fully compliant.
✨ 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,11 @@ | |||
| .terraform/ | |||
| *.tfstate | |||
| *.tfstate.* | |||
There was a problem hiding this comment.
Backend config must match checklist item #9: use mate-azure-task-12 as resource_group_name and yourstorageaccount as storage_account_name, with container tfstate and key terraform.tfstate. The current values (tfstate-rg, tfstate2426127369) do not comply.
| .terraform/ | ||
| *.tfstate | ||
| *.tfstate.* | ||
| .terraform.tfstate.lock.info |
There was a problem hiding this comment.
The task requires location to be uksouth (checklist #16); your default is denmarkeast, which does not match the specification.
| @@ -0,0 +1,43 @@ | |||
| # This file is maintained automatically by "terraform init". | |||
| # Manual edits may be lost in future updates. | |||
|
|
|||
There was a problem hiding this comment.
Task checklist item #9 and description require the backend to use resource group mate-azure-task-12 and storage account yourstorageaccount, not tfstate-rg. Update this to match the exact required values.
| # This file is maintained automatically by "terraform init". | ||
| # Manual edits may be lost in future updates. | ||
|
|
||
| provider "registry.terraform.io/hashicorp/azurerm" { |
There was a problem hiding this comment.
The storage account name for the backend must be yourstorageaccount per the description/checklist, not tfstate2426127369. This is important for remote state configuration compliance.
| "zh:5f211d8ec3f2e1f414870d9584bfe26e6995560ef81c748f8447a48164767398", | ||
| "zh:78d5eefdd9e494defcb3c68d282b8f96630502cac21d1ea161f53cfe9bb483b3", | ||
| "zh:7b547fd16216761ef86efc3ed516ac5ac0c5c42b7c7eb24a08cef2d93f69ed5e", | ||
| "zh:7e7c0679daf2a382151d05068c8c3f0dae6b7b7dccf818827b73dd08638df2ef", |
| # This file is maintained automatically by "terraform init". | ||
| # Manual edits may be lost in future updates. | ||
|
|
||
| provider "registry.terraform.io/hashicorp/azurerm" { |
There was a problem hiding this comment.
The location variable is required to default to "uksouth" (checklist item #16), but here it defaults to "denmarkeast". Adjust this default to meet the task specification.
| "zh:7b547fd16216761ef86efc3ed516ac5ac0c5c42b7c7eb24a08cef2d93f69ed5e", | ||
| "zh:7e7c0679daf2a382151d05068c8c3f0dae6b7b7dccf818827b73dd08638df2ef", | ||
| "zh:8089dec888a8038b9b4fb23b3df7e1057293dbc5b60b42cc47ff690d69d4b61b", | ||
| "zh:c51f15a031edfd6f23ce8ced3446ca7f8d8d647e2499890d7d5d10d5016d7257", |
There was a problem hiding this comment.
The task description and checklist refer to an ssh_key_public variable with a specific value. Here you define ssh_public_key instead, which may cause mismatch with expected naming; consider renaming to ssh_key_public to align with the requirements.
| @@ -0,0 +1,8 @@ | |||
| terraform { | |||
| backend "azurerm" { | |||
| resource_group_name = "tfstate-rg" | |||
There was a problem hiding this comment.
Backend config does not match checklist item #9: resource_group_name must be mate-azure-task-12 and storage_account_name must be yourstorageaccount, but here different values are used. Update these to the specified ones so the remote state backend complies with the task description.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "tfstate-rg" | ||
| storage_account_name = "tfstate2426127369" |
There was a problem hiding this comment.
Similarly, storage_account_name should be yourstorageaccount per the backend requirements, not this hard-coded value. Adjust it to satisfy checklist item #9.
| vm_name = var.vm_name | ||
| vm_size = var.vm_size | ||
| admin_username = var.admin_username | ||
| ssh_public_key = var.ssh_public_key |
There was a problem hiding this comment.
The compute module correctly wires in the network module outputs and VM parameters, but note that the checklist expects a variable named ssh_key_public with a specific default value. Here you pass ssh_public_key, which may not match the task’s exact variable naming requirement.
| terraform { | ||
| required_providers { |
There was a problem hiding this comment.
NIC naming via ${var.vm_name}-nic satisfies the requirement that the network interface be named ${var.vm_name}-nic (checklist #36), so this part is fine.
| network_security_group_name = var.network_security_group_name | ||
| public_ip_address_name = var.public_ip_address_name | ||
| dns_label = var.dns_label | ||
| } |
There was a problem hiding this comment.
The VM is using Ubuntu 22.04 via Jammy LTS image, which is correct in practice, but the checklist calls for image Ubuntu2204. If the validation is strict, you might need to ensure your configuration matches the expected label or document the mapping from Ubuntu2204 to this Canonical image.
| resource_group_name = azurerm_resource_group.main.name | ||
| location = azurerm_resource_group.main.location | ||
| storage_account_name = var.storage_account_name | ||
| } | ||
|
|
||
| module "compute" { | ||
| source = "./modules/compute" | ||
|
|
||
| resource_group_name = azurerm_resource_group.main.name | ||
| location = azurerm_resource_group.main.location |
There was a problem hiding this comment.
| resource "azurerm_virtual_machine_extension" "app" { | ||
| name = "install-app" | ||
| virtual_machine_id = azurerm_linux_virtual_machine.vm.id | ||
| publisher = "Microsoft.Azure.Extensions" | ||
| type = "CustomScript" | ||
| type_handler_version = "2.1" | ||
|
|
||
| settings = jsonencode({ | ||
| fileUris = [var.install_script_url] | ||
| commandToExecute = "bash install-app.sh" |
There was a problem hiding this comment.
| username = var.admin_username | ||
| public_key = var.ssh_public_key | ||
| } | ||
|
|
There was a problem hiding this comment.
The task description and checklist refer to a variable ssh_key_public and an SSH key named linuxboxsshkey. Here you use ssh_public_key instead, which is a naming mismatch. Consider renaming this variable to ssh_key_public and aligning with the required SSH key name.
| description = "ID of the subnet the network interface attaches to" | ||
| type = string | ||
| } | ||
|
|
There was a problem hiding this comment.
The VM image is clearly Ubuntu 22.04 (Jammy), but the checklist specifies Ubuntu2204 as the image. If the evaluation expects that exact label, you may need to adjust the image reference or at least confirm that this mapping is acceptable.
| type = string | ||
| } | ||
|
|
||
| variable "ssh_public_key" { |
There was a problem hiding this comment.
The task description expects a variable named ssh_key_public with a specific content (your-public-key-content) and mentions an SSH key linuxboxsshkey. Here you use ssh_public_key instead, which may cause mismatch with the required naming and key identity.
| name = var.public_ip_address_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Static" |
| output "storage_account_name" { | ||
| description = "Generated name of the storage account" | ||
| value = azurerm_storage_account.storage.name | ||
| } | ||
|
|
||
| output "storage_container_name" { | ||
| description = "Name of the storage container" | ||
| value = azurerm_storage_container.container.name |
There was a problem hiding this comment.
The outputs for storage are fine and match the requirements: you expose the generated storage account name and the task-artifacts container as expected.
| output "storage_account_name" { | ||
| description = "Generated name of the storage account" | ||
| value = azurerm_storage_account.storage.name |
There was a problem hiding this comment.
Using module.network.public_ip_fqdn to build the app_url is a good way to surface the app endpoint; this aligns with the requirement to be able to verify the app in a browser.
| @@ -0,0 +1,9 @@ | |||
| output "storage_account_name" { | |||
| description = "Generated name of the storage account" | |||
| value = azurerm_storage_account.storage.name | |||
There was a problem hiding this comment.
The tfvars file overrides ssh_public_key, but the checklist requires a variable named ssh_key_public with content "your-public-key-content". This mismatch in variable name and expected value means your configuration does not satisfy checklist item #26.
| @@ -0,0 +1,14 @@ | |||
| variable "resource_group_name" { | |||
| description = "Name of the resource group for the storage resources" | |||
| type = string | |||
| @@ -0,0 +1,14 @@ | |||
| variable "resource_group_name" { | |||
| description = "Name of the resource group for the storage resources" | |||
| type = string | |||
There was a problem hiding this comment.
The task description defines a variable ssh_key_public with value "your-public-key-content". Here, you use ssh_public_key and set it in terraform.tfvars, which does not match the required name/value (checklist #26). Consider renaming the variable and aligning its default/value.
| variable "resource_group_name" { | ||
| description = "Name of the resource group for the storage resources" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The location variable default is "denmarkeast", but checklist item #16 requires uksouth as the location. Update this default to comply with the requirement.
| @@ -0,0 +1,9 @@ | |||
| output "app_url" { | |||
| description = "URL to open the ToDo List application in a browser" | |||
| value = "http://${module.network.public_ip_fqdn}:8080" | |||
There was a problem hiding this comment.
The checklist expects a variable named ssh_key_public with a specific content ("your-public-key-content"). Here you provide ssh_public_key in tfvars, which does not match the required variable name and value (checklist item #26).
| @@ -0,0 +1,3 @@ | |||
| storage_account_name = "matetaskvolodymyr" | |||
| install_script_url = "https://raw.githubusercontent.com/volodymyrlp/devops_todolist_terraform_task/main/install-app.sh" | |||
| ssh_public_key = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQCn9u247Nw8JycJeA/hx0l0lAh8EA5QI1Ewrmq2AOyh/UMqWNY2PhMmNrPsWKa2MhJS3vHo2u19c5QBh2e8mqYoLg43T7CgL6w/ysL79xychJTAdI4iKEHFD3/vFpdWYDPkisYv+1UKDG6YF/hkSeJUEfuO+TrYlVCiLyJPYbAcWrFPMsS6kXhQXP40Zzp910FzxkHexIepE5L+/O00mCarj45vEOiAsPQgLqa1vi1hYMfZ1cz6Ift+PkpFFkApDzJiBnzGyK/ArEqn0tYMH/AvEQJkUSPP5vLSvY5DTk6aZ8bHVxNd+qOMmx6rjWiwoovkHX0GGeDZh8wWZLxGu+KqHXBWf40HLrqri3754ErowNN7hHETt97suFT7kWgQywtez8wY5aOdsLVoHALyLW+vBOnmWAMtYXJJgOJqdoDEewMJH7BnmkPs0CJQorLUnTozoxfVECuec2DKhPDhbN6Rj2GT3hK32PjPI78pILiWeSV38PhQMTKk28K3KyOfa+POWIrDsVF1DKqNwZQnbDTWo0yQktHZYsJaVu25xgvG38kM8LtfQhtJhFS7thc5dYbBHnZVxSRyPiExTi/zSS4t/jHpPC5HhkH1tX/hTFD46uJCeMMYkQpEpSFgnvwMYPQSdLbgJa8DmqGS2g0VU1RneWzCB6P6p6BYCO9M84LeVw== macbookair@MacBookAir" | |||
There was a problem hiding this comment.
terraform.tfvars defines ssh_public_key, but the task description and checklist refer to a variable ssh_key_public with a specific content (your-public-key-content). To comply strictly, you should rename this variable and align its value with the requirement.
| variable "location" { | ||
| description = "Azure region for all resources" | ||
| type = string | ||
| default = "denmarkeast" |
There was a problem hiding this comment.
The location default must be "uksouth" per checklist item #16; currently it is "denmarkeast". Update this to meet the required region.
…, dynamic public IP, ssh_key_public
|
@mateacademy-ai-mentor thanks for the review! Addressed all requested changes:
CI |
Deploying Todo List Application With Terraform
Modular Terraform configuration that provisions the full stack and deploys the ToDo List app via a CustomScript VM extension.
Modules
10.0.0.0/16, subnetdefault10.0.0.0/24, NSGdefaultnsg(inbound 22 + 8080) associated with the subnet, public IPlinuxboxpip(dynamic) with DNS labelmatetask<random>.matebox-nic, VMmatebox(Standard_B1s, Ubuntu 22.04), CustomScript extension runninginstall-app.sh.task-artifacts.Remote state backend in Azure Blob Storage (
backend.tf): RGmate-azure-task-12, containertfstate, keyterraform.tfstate.Verification
Deployed successfully; the application responds with HTTP 200 at
http://<public-ip-fqdn>:8080(page title Djodolist).Successful workflow run
https://github.com/mate-academy/devops_todolist_terraform_task/actions/runs/28548842536 ✅