Solution#72
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core checklist requirement for the remote backend is not met, which is considered a critical blocker for this task. In backend.tf (and related backend settings you duplicated), storage_account_name is set to "mateterraformtask12", but the task explicitly requires storage_account_name = "yourstorageaccount" for the backend configuration [CHECKLIST ITEM #36]; this must be corrected and the corresponding storage account and tfstate container must actually exist in the mate-azure-task-12 resource group. Functionally, your modules for network, compute, and storage are very solid: the VM matebox, size Standard_B1s, Ubuntu 22.04 image, NIC, NSG, VNet, subnet, dynamic public IP, and CustomScript extension running install-app.sh all match the described architecture and behave as required.
The critical fix you need is:
- Update every backend configuration (e.g., in
backend.tfand any variables/lock-related references) sostorage_account_nameis exactly"yourstorageaccount", then ensure that storage account and thetfstatecontainer are created in Azure and accessible to Terraform.
Minor, non-blocking improvements you might consider afterwards: change the dns_label base in terraform.tfvars from "matetask12" to "matetask" to align literally with the spec, and, if your reviewers are strict, document that your Canonical Ubuntu 22.04 image corresponds to the requested Ubuntu2204 identifier. Once you fix the backend name and confirm the remote state storage exists, your solution should fully satisfy the task requirements, and you’re very close already—nice work on the module structure and VM/app configuration.
✨ 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". | |||
| # Manual edits may be lost in future updates. | |||
|
|
|||
There was a problem hiding this comment.
Per checklist item #36, the backend must use storage_account_name = "yourstorageaccount". Using "mateterraformtask12" here doesn’t match the task description and should be updated to the required value.
| @@ -0,0 +1,8 @@ | |||
| terraform { | |||
| backend "azurerm" { | |||
| storage_account_name = "mateterraformtask12" | |||
There was a problem hiding this comment.
Checklist item #36 requires the backend storage_account_name to be "yourstorageaccount", but this is currently set to "mateterraformtask12". Update this value (and ensure the corresponding storage account exists) to match the task description exactly.
| @@ -0,0 +1,50 @@ | |||
| terraform { | |||
| required_providers { | |||
| azurerm = { | |||
There was a problem hiding this comment.
The backend configuration must match checklist item #36: storage_account_name is required to be "yourstorageaccount", but here it is set to a different value. Update this to the specified name and ensure the corresponding storage account exists.
| source_image_reference { | ||
| publisher = "Canonical" | ||
| offer = "0001-com-ubuntu-server-jammy" | ||
| sku = "22_04-lts" | ||
| version = "latest" |
There was a problem hiding this comment.
This VM configuration correctly uses vm_name and vm_size to satisfy the required name (matebox) and size (Standard_B1s). The Ubuntu Jammy 22.04 image also matches the Ubuntu2204 requirement functionally, so you don’t need to change this unless your reviewers insist on a different image notation.
| resource "azurerm_virtual_machine_extension" "main" { | ||
| name = "install-extension" | ||
| virtual_machine_id = azurerm_linux_virtual_machine.main.id | ||
| publisher = "Microsoft.Azure.Extensions" | ||
| type = "CustomScript" | ||
| type_handler_version = "2.0" | ||
|
|
||
| protected_settings = jsonencode({ | ||
| "script" = base64encode(file("${path.root}/install-app.sh")) | ||
| }) |
There was a problem hiding this comment.
Using the CustomScript extension with install-app.sh via protected_settings fulfills the VM extension requirements (type CustomScript and executing install-app.sh). Ensure the script path and contents remain consistent so the ToDo app deploys correctly.
| @@ -0,0 +1,9 @@ | |||
| variable "location" { | |||
| type = string | |||
| description = "The location of VM" | |||
There was a problem hiding this comment.
This backend block is functionally valid, but checklist item #36 requires storage_account_name to be exactly "yourstorageaccount". Update this value to match the task description and ensure that storage account actually exists in the mate-azure-task-12 resource group.
| @@ -0,0 +1,3 @@ | |||
| output "resource_group_id" { | |||
| value = azurerm_resource_group.main.id | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
This backend block works technically, but checklist item #36 requires storage_account_name = "yourstorageaccount". Update this value and ensure the corresponding storage account and tfstate container exist in Azure.
| @@ -0,0 +1,12 @@ | |||
| location = "uksouth" | |||
| resource_group_name = "mate-azure-task-12" | |||
| virtual_network_name = "vnet" | |||
There was a problem hiding this comment.
The backend must use storage_account_name = "yourstorageaccount" according to checklist item #36; using a different name here deviates from the task requirements and should be corrected.
| @@ -0,0 +1,59 @@ | |||
| variable "location" { | |||
| type = string | |||
| description = "The location of VM" | |||
There was a problem hiding this comment.
The backend configuration must use the exact parameters specified in the task description. Checklist item #36 requires storage_account_name = "yourstorageaccount", but this is currently set to a different value. Update this to "yourstorageaccount" and ensure the corresponding storage account and tfstate container exist.
| } | ||
|
|
||
| variable "virtual_network_name" { | ||
| type = string |
There was a problem hiding this comment.
The specification for dns_label is matetask (with a random number appended by your script/module). Using "matetask12" here still works but doesn’t strictly match checklist item #44; consider changing it to "matetask" for full compliance.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your Terraform solution is approved because it implements all required features and matches the task checklist. You correctly configured the remote backend to use the mate-azure-task-12 resource group, yourstorageaccount storage account, tfstate container, and terraform.tfstate key, and your root main.tf cleanly wires the network, compute, and storage modules together. The network module creates the specified VNet, subnet, NSG rules (22/80/8080), and a dynamic public IP with the matetask-<random> DNS label, while the compute module provisions the matebox VM (Ubuntu 22.04, Standard_B1s, SSH key-based access) plus a CustomScript extension that runs install-app.sh. The storage module correctly creates a separate storage account and the task-artifacts container, and your install-app.sh script installs dependencies, clones your fork of the ToDo app, and configures the systemd service, satisfying the app deployment requirement. The remaining concerns (actual existence of the backend storage account/container and runtime verification of the app in a browser) are operational, not code-level blockers, so they don’t prevent completion of this task. As a minor improvement, you could add root outputs for the public IP and DNS label to make manual verification easier, but this is optional. Overall, this is a solid, well-structured implementation that meets the project standards.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
Reference to successful workflow run:
https://github.com/nichelangeloo/devops_todolist_terraform_task/actions/runs/28325938050/job/83915857336