Develop Terraform Infrastructure#74
Conversation
Add Network to main.tf
Add outputs to Network Module
Add compute to main.tf, fix install-app.sh script
Add storage to main.tf Reformat modules to use existing group resource
azurerm_linux_virtual_machine
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical requirement mismatches that block full acceptance of the task.
Critical issues that must be fixed:
- The backend configuration in
backend.tfusesstorage_account_name = "ands9storageaccount", but the task explicitly requiresstorage_account_name = "yourstorageaccount"[CHECKLIST ITEM #12/#19]; update this value and align any bootstrap/storage creation with that exact name. - The public IP in
modules/network/main.tfis defined withallocation_method = "Static", while the assignment requires a dynamic public IP forlinuxboxpip[CHECKLIST ITEM #10/#16]; change this to"Dynamic"so the networking configuration matches the specification. - In
modules/compute/variables.tf, thevm_sizevariable still defaults to"Standard_D2s_v3", which contradicts the requiredStandard_B1sVM size [CHECKLIST ITEM #8/#17]; set this default to"Standard_B1s"so the compute module itself is compliant, not just the root module.
Important but non‑blocking improvements:
- Your CustomScript extension currently uses a
scriptfield with base64-encoded content; it’s safer to follow Azure’s documented pattern withcommandToExecute(and optionallyfileUris) to guaranteeinstall-app.shruns correctly. - The
vm_ip_configoutput that ultimately referencesazurerm_linux_virtual_machine.main.public_ip_addressmay not work with newer providers; consider relying solely on the public IP output from the network module.
Overall, your structure, module usage, and app deployment logic are very solid; once you fix the backend name, public IP allocation, and compute module size default, your solution will be in great shape and should fully satisfy the checklist.
✨ 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,9 @@ | |||
| terraform { | |||
| backend "azurerm" { | |||
| storage_account_name = "ands9storageaccount" | |||
There was a problem hiding this comment.
The task requires the backend storage account name to be yourstorageaccount (checklist item #12), but this is set to ands9storageaccount. Update this to yourstorageaccount to match the description and ensure the remote state backend configuration is compliant.
| @@ -0,0 +1,12 @@ | |||
| # COMMON VARIABLES | |||
| variable "resource_group_name" { | |||
| description = "Name of the resource group" | |||
There was a problem hiding this comment.
|
|
||
| data "azurerm_resource_group" "main" { | ||
| name = var.resource_group_name | ||
| } |
There was a problem hiding this comment.
The requirement states the VM image must be Ubuntu 22.04 (Ubuntu2204). While your source_image_reference is correct for Ubuntu 22.04 LTS, double-check that this mapping is acceptable for the task; if they expect a specific shorthand or image alias, you may need to adjust, but functionally this is Ubuntu 22.04.
|
|
||
| resource "azurerm_resource_group" "example" { | ||
| name = var.resource_group_name | ||
| location = var.resource_group_location |
There was a problem hiding this comment.
The assignment requires the VM size to be Standard_B1s (checklist items #8 and #17). Here, the VM size is driven by var.vm_size, and while the root variables default to Standard_B1s, make sure you don’t override this with a different value in terraform.tfvars or elsewhere, otherwise you’ll violate the requirement.
| extension_name = "CustomScript" | ||
| path_to_script = "install-app.sh" |
There was a problem hiding this comment.
The compute module correctly configures a CustomScript extension pointing to install-app.sh, which meets the requirement to deploy the ToDo app via VM extension. Ensure that install-app.sh is in the expected relative location when Terraform runs so the file(...) function resolves properly.
| name = var.pip_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.resource_group_location | ||
| allocation_method = "Static" |
| description = "Name of the subnet" | ||
| type = string | ||
| default = "default" | ||
| } |
| } | ||
| variable "v_net_address_prefix" { | ||
| description = "Virtual network address prefix" | ||
| type = list(string) | ||
| default = ["10.0.0.0/16"] |
| variable "container_name" { | ||
| type = string | ||
| default = "storage_container" |
There was a problem hiding this comment.
The task requires the storage container to be named task-artifacts (checklist items #11 and #18), but the default here is "storage_container". Since you already pass "task-artifacts" from the root module, behavior is correct, but for strict compliance and clarity, consider changing this default to "task-artifacts".
|
|
||
| variable "container_name" { |
There was a problem hiding this comment.
vm_ip_config is wired to module.compute.vm_public_ip_address, which in turn uses azurerm_linux_virtual_machine.main.public_ip_address. In recent azurerm versions, the VM resource doesn’t expose a public_ip_address attribute directly; you typically get that from the NIC/public IP resources. This might cause Terraform errors or return null; consider relying on the network module’s public IP output instead.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still one critical blocker related to the remote state backend that violates the explicit task requirements. Your backend.tf correctly configures the backend to use storage_account_name = "yourstorageaccount" and container tfstate, but in bootstrap/main.tf (around the backend_storage module call) you still create a storage account named "ands9storageaccount", so the backend points to a storage account that Terraform never creates; this breaks the requirement that the backend storage account/container actually exist with the exact configured names. To fix this, change the bootstrap module invocation so that storage_account_name is exactly "yourstorageaccount" and ensure it creates a tfstate container in the mate-azure-task-12 resource group. As a small consistency improvement, you should also update the root variables.tf default storage_account_name from "ands9prostorrage" to "yourstorageaccount" (or always override it) and correct the typo in ssh_key_public from "your-public-key-conten" to "your-public-key-content". Aside from this backend naming mismatch, your modules, VM configuration (Ubuntu 22.04, Standard_B1s, matebox), dynamic public IP linuxboxpip, DNS label matetask-<random>, and CustomScript execution of install-app.sh all look solid and aligned with the task. Once you align the bootstrap-created storage account with the backend configuration, your solution should fully meet the assignment’s requirements.
✨ 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,44 @@ | |||
| # Local .terraform directories | |||
| .terraform/ | |||
|
|
|||
There was a problem hiding this comment.
The backend configuration here correctly uses storage_account_name = "yourstorageaccount", container_name = "tfstate", and resource_group_name = "mate-azure-task-12", which matches the task requirements. Just make sure the storage account with this exact name is actually created by your bootstrap or main configuration (see the bootstrap file).
| # Ignore override files as they are usually used to override resources locally and so | ||
| # are not checked in | ||
| override.tf | ||
| override.tf.json |
There was a problem hiding this comment.
This module call provisions a storage account named "ands9storageaccount", but the backend configuration requires the storage account to be named "yourstorageaccount". To satisfy the remote state requirements, update this value so the created storage account name matches the backend (yourstorageaccount) and the backend can actually use it.
| @@ -0,0 +1,9 @@ | |||
| terraform { | |||
| backend "azurerm" { | |||
| storage_account_name = "yourstorageaccount" | |||
There was a problem hiding this comment.
The backend configuration correctly uses storage_account_name = "yourstorageaccount", which matches the explicit requirement for the remote state backend.
| source = "../modules/storage" | ||
| resource_group_name = azurerm_resource_group.example.name | ||
| resource_group_location = azurerm_resource_group.example.location | ||
| storage_account_name = "ands9storageaccount" |
There was a problem hiding this comment.
This bootstrap module is creating a storage account named "ands9storageaccount", but your backend.tf (and the task description) require the backend storage account to be named exactly "yourstorageaccount". Update this value so the bootstrap creates yourstorageaccount, ensuring the remote state backend actually exists with the expected name (checklist item about backend config and existing storage).
| mkdir /app | ||
| # make sure to uncomment the line bellow and update the link with your GitHub username | ||
| # git clone https://github.com/<your-gh-username>/azure_task_12_deploy_app_with_vm_extention.git | ||
| git clone https://github.com/AndS9/devops_todolist_terraform_task.git |
There was a problem hiding this comment.
The script correctly installs dependencies, clones the Todo app repository, copies the app files into /app, sets up the systemd service, and starts/enables it. This satisfies the requirement that the VM extension deploys the ToDo List app by executing install-app.sh. Just ensure this repo URL is correct for your fork if the instructions expect using your own GitHub username.
|
|
||
| provider "azurerm" { | ||
| features { | ||
|
|
There was a problem hiding this comment.
This git clone URL uses your own GitHub username. The task description suggests replacing it with the student’s username, but functionally this still satisfies the requirement to deploy the provided ToDo app; only adjust if you intend this repo to be student-agnostic.
| extension_name = "CustomScript" | ||
| path_to_script = "install-app.sh" |
There was a problem hiding this comment.
The CustomScript extension is correctly referenced, but you’re using protected_settings.script with a base64-encoded file. Azure typically expects settings with commandToExecute (and optionally fileUris); if you encounter deployment issues, consider switching to the documented pattern so install-app.sh is executed reliably.
| } | ||
|
|
||
| resource "azurerm_virtual_machine_extension" "example" { | ||
| name = var.extension_name | ||
| 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.module}/../../${var.path_to_script}")) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The root configuration correctly wires the compute module and passes vm_name, vm_size, ssh_public_key_content, and the path to install-app.sh, satisfying the requirement to use modules and to deploy the app via a CustomScript extension.
| resource "azurerm_network_interface" "main" { | ||
| name = "${var.vm_name}-nic" |
There was a problem hiding this comment.
The NIC is correctly named ${var.vm_name}-nic and associated with the provided subnet and public IP, matching the compute module requirements.
| resource "azurerm_linux_virtual_machine" "main" { | ||
| name = var.vm_name | ||
| location = var.resource_group_location | ||
| resource_group_name = var.resource_group_name | ||
| network_interface_ids = [azurerm_network_interface.main.id] | ||
| size = var.vm_size |
There was a problem hiding this comment.
The VM uses size = var.vm_size, and with the module default set to "Standard_B1s", this now aligns with the required VM size.
| resource "azurerm_virtual_machine_extension" "example" { | ||
| name = var.extension_name | ||
| 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.module}/../../${var.path_to_script}")) |
There was a problem hiding this comment.
This CustomScript extension correctly references the VM and uses the script file from the repo. While Azure docs usually show commandToExecute, using the script field here still satisfies the requirement to use CustomScript to execute install-app.sh.
| resource "azurerm_network_interface" "main" { | ||
| name = "${var.vm_name}-nic" |
There was a problem hiding this comment.
The output exposes the VM ID, which is consistent with how the root module references module.compute.vm_id.
| variable "vm_size" { | ||
| description = "Size of VM" | ||
| type = string | ||
| default = "Standard_B1s" |
There was a problem hiding this comment.
The vm_size default is now "Standard_B1s", which matches the required VM size for the compute module and fixes the earlier mismatch noted in the checklist.
| } | ||
|
|
||
| variable "vm_name" { | ||
| description = "Virtual Machine Name" | ||
| type = string |
There was a problem hiding this comment.
The public IP resource now uses allocation_method = "Dynamic" for linuxboxpip, which satisfies the requirement that the public IP must be dynamically allocated.
| } | ||
|
|
||
| resource "azurerm_network_security_group" "main" { | ||
| name = var.network_SG_name |
There was a problem hiding this comment.
The vm_size variable default is correctly set to "Standard_B1s", resolving the previous requirement mismatch and ensuring the compute module itself enforces the required VM size.
| resource "azurerm_public_ip" "mainIP" { | ||
| name = var.pip_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.resource_group_location | ||
| allocation_method = "Dynamic" |
There was a problem hiding this comment.
The public IP now uses allocation_method = "Dynamic", which aligns with the task requirement for linuxboxpip and fixes the earlier static allocation issue.
| resource "azurerm_virtual_network" "main" { | ||
| name = var.network_name | ||
| address_space = ["10.0.0.0/16"] | ||
| location = var.resource_group_location | ||
| resource_group_name = var.resource_group_name | ||
| } | ||
|
|
||
| resource "azurerm_subnet" "internal" { | ||
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.main.name | ||
| address_prefixes = ["10.0.0.0/24"] |
There was a problem hiding this comment.
The VNet and subnet are hardcoded to 10.0.0.0/16 and 10.0.0.0/24 here, which matches the required address spaces. Just be aware that the v_net_address_prefix and subnet_address_prefix variables are not used in these resources; if you intended them to be configurable, you could plug them in instead of literals.
| output "public_ip_id" { | ||
| value = azurerm_public_ip.mainIP.id | ||
| } | ||
| output "public_ip" { | ||
| value = azurerm_public_ip.mainIP.ip_address | ||
| } | ||
| output "public_domain" { | ||
| value = azurerm_public_ip.mainIP.domain_name_label | ||
| } | ||
| output "NSG_id" { | ||
| value = azurerm_network_security_group.main.id |
There was a problem hiding this comment.
The public IP and NSG outputs are correctly exposed for use by the root module, enabling you to wire the compute module and outputs as required.
| description = "Name of the subnet" | ||
| type = string | ||
| default = "default" | ||
| } |
There was a problem hiding this comment.
The vm_size default is correctly set to "Standard_B1s", which matches the required VM size and fixes the previous mismatch in the compute module defaults (checklist item about VM size).
| } | ||
| variable "v_net_address_prefix" { | ||
| description = "Virtual network address prefix" | ||
| type = list(string) | ||
| default = ["10.0.0.0/16"] |
There was a problem hiding this comment.
The public IP resource now uses allocation_method = "Dynamic" for linuxboxpip, which satisfies the requirement that the public IP be dynamically allocated (this fixes the earlier issue where it was static).
| } | ||
| variable "v_net_address_prefix" { | ||
| description = "Virtual network address prefix" | ||
| type = list(string) | ||
| default = ["10.0.0.0/16"] | ||
| } | ||
| variable "subnet_name" { |
There was a problem hiding this comment.
The DNS label is parameterized via var.dns_label, and in the root module you append a random suffix ("${var.dns_label}-${random_string.suffix.result}"), which matches the requirement to generate the DNS label by concatenating matetask and a random number.
| resource "azurerm_storage_account" "example" { | ||
| name = var.storage_account_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.resource_group_location | ||
| account_tier = "Standard" | ||
| account_replication_type = "LRS" | ||
| } | ||
|
|
||
| resource "azurerm_storage_container" "example" { | ||
| name = var.container_name |
There was a problem hiding this comment.
This module correctly creates a storage account and container using the provided names. Just make sure that when you call this module for backend provisioning, you pass storage_account_name = "yourstorageaccount" and container_name = "tfstate" to match the backend configuration.
| output "storage_account_name" { | ||
| value = azurerm_storage_account.example.name | ||
| } | ||
| output "storage_account_id" { |
There was a problem hiding this comment.
This module correctly parameterizes the storage account name and uses the passed resource_group_name and resource_group_location, which allows you to reuse it both for backend state and for the task-artifacts container.
| output "storage_account_name" { | ||
| value = azurerm_storage_account.example.name | ||
| } | ||
| output "storage_account_id" { | ||
| value = azurerm_storage_account.example.id | ||
| } | ||
| output "container_id" { | ||
| value = azurerm_storage_container.example.id |
There was a problem hiding this comment.
These outputs are well-chosen and used in the root outputs to expose storage information; no changes are required here for the assignment.
| type = string | ||
| default = "Poland Central" |
There was a problem hiding this comment.
This storage container will be named based on var.container_name; in the root module you correctly override this to "task-artifacts", which meets the requirement that the storage module create a container named task-artifacts.
| variable "container_name" { | ||
| type = string |
There was a problem hiding this comment.
Root outputs correctly expose the storage container ID from the storage module, which can be useful for verifying that task-artifacts was created as required.
| output "resource_group" { | ||
| value = data.azurerm_resource_group.main.name |
There was a problem hiding this comment.
This module correctly creates a storage account and container, but note that the actual name for the backend storage account must be "yourstorageaccount" according to the task. Make sure that when you invoke this module for the backend, you pass storage_account_name = "yourstorageaccount" rather than the current custom names used elsewhere.
| output "container_taskartifacts_id" { | ||
| value = module.storage.container_id | ||
| } |
There was a problem hiding this comment.
The container name default is "storage_container", but in root main.tf you correctly override it with "task-artifacts" for the assignment; that’s fine as long as the backend-related container for tfstate is also created somewhere with the required name.
|
|
||
| output "vm_id" { | ||
| value = module.compute.vm_id |
There was a problem hiding this comment.
These outputs correctly expose the container id and are used in root outputs.tf to provide container_taskartifacts_id, which meets the requirement to have a task-artifacts container and expose it.
| @@ -0,0 +1,124 @@ | |||
| # COMMON VARIABLES | |||
| variable "resource_group_name" { | |||
| description = "Name of the resource group" | |||
There was a problem hiding this comment.
The backend is correctly configured to use storage_account_name = "yourstorageaccount", but your bootstrap still creates a different storage account name. To ensure the backend points to a real storage account and satisfy the remote state requirement, update the bootstrap so it also creates "yourstorageaccount".
| } | ||
| variable "vnet_address_prefix" { | ||
| description = "Virtual network address prefix" | ||
| type = list(string) |
There was a problem hiding this comment.
Here you still create the backend storage account as "ands9storageaccount", which does not match the required "yourstorageaccount" used by backend.tf. Change this value to "yourstorageaccount" so the backend storage account name is consistent with the assignment and backend configuration.
| variable "storage_account_name" { | ||
| type = string | ||
| default = "ands9prostorrage" |
There was a problem hiding this comment.
The default storage account name "ands9prostorrage" does not match the required backend storage account name "yourstorageaccount". For clarity and compliance, consider changing this default to "yourstorageaccount" or ensure you override it appropriately when deploying.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as meeting the educational goals of the assignment and showing solid understanding of Terraform modules, Azure resources, and VM extensions. Your backend and bootstrap setup now correctly create and reference mate-azure-task-12, yourstorageaccount, and the tfstate container, and your network, compute, and storage modules all align well with the required names, prefixes, and behaviors. One educational note: your VM name is controlled by var.vm_name (default matebox); to strictly match the spec you could hardcode name = "matebox" or limit changing this variable, but your current approach still works correctly. Another minor improvement opportunity is in install-app.sh and the CustomScript extension: adding a simple directory-existence check before git clone would make re-runs more robust, and using commandToExecute is the more typical pattern, though your current base64 approach is valid. Overall, you’ve done a great job wiring modules together, configuring networking and security rules, and deploying the ToDo app via the VM extension—this is a strong, realistic Terraform solution. Keep refining small details like strict naming and idempotent scripts, but you’re clearly on the right track and your work is approved for the assignment.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| mkdir /app | ||
| # make sure to uncomment the line bellow and update the link with your GitHub username | ||
| # git clone https://github.com/<your-gh-username>/azure_task_12_deploy_app_with_vm_extention.git | ||
| git clone https://github.com/AndS9/devops_todolist_terraform_task.git |
There was a problem hiding this comment.
For the assignment, this hard-coded clone URL still points to your own GitHub username. The task text asks to update it with your GitHub username; if this repo is meant to be reused by others, they’ll need to change it, but for your submission, this is fine as long as the URL matches the app repo you control.
| } | ||
| /* | ||
| I CAN"T USE IT IN MAIN, BECAUSE IT MUST BE CREATED WITH STORAGE ACC | ||
| BEFORE BACKEND for TERRAFORM TO PROPERLY START |
There was a problem hiding this comment.
The assignment explicitly says the VM should be named matebox, but here the name is driven purely by var.vm_name; while the root default is "matebox", the module doesn’t enforce it. Consider hardcoding "matebox" here (or constraining vm_name) to exactly satisfy the requirement.
| extension_name = "CustomScript" | ||
| path_to_script = "install-app.sh" |
There was a problem hiding this comment.
This CustomScript extension wiring correctly points at install-app.sh, but note that using protected_settings.script with base64-encoded file content is slightly unusual; typically commandToExecute is used. It still satisfies the requirement to execute install-app.sh, so this is acceptable, just be sure it behaves as expected in your environment.
|
|
||
| provider "azurerm" { | ||
| features { | ||
|
|
There was a problem hiding this comment.
Re-running this script when the devops_todolist_terraform_task directory already exists could cause git clone to fail, which might make the extension show as failed on reapply. The task doesn’t require idempotency, but adding a simple if [ ! -d ... ] guard would make the script more robust.
|
|
||
| resource "azurerm_network_interface_security_group_association" "main" { | ||
| network_interface_id = azurerm_network_interface.main.id | ||
| network_security_group_id = var.NSG_id |
There was a problem hiding this comment.
The git clone URL still points to your own GitHub username. The task description suggests updating this link with the student’s GitHub username when they reuse the template; if this repo is your own submission, this is fine, but if it’s meant as a general solution, consider parameterizing or documenting that this needs to be changed.
| } | ||
|
|
||
| output "public_ip_id" { | ||
| value = azurerm_public_ip.mainIP.id | ||
| } | ||
| output "public_ip" { | ||
| value = azurerm_public_ip.mainIP.ip_address | ||
| } | ||
| output "public_domain" { | ||
| value = azurerm_public_ip.mainIP.domain_name_label |
There was a problem hiding this comment.
You define v_net_address_prefix and subnet_address_prefix variables, but here you hard-code the address space to 10.0.0.0/16 and 10.0.0.0/24. That matches the assignment requirements, but if you intend these prefixes to be configurable, consider using the variables instead of literals.
| @@ -0,0 +1,86 @@ | |||
| variable "resource_group_name" { | |||
| description = "Name of the resource group" | |||
| type = string | |||
There was a problem hiding this comment.
You define v_net_address_prefix and subnet_address_prefix variables, but in main.tf you’re hardcoding the address space/prefixes instead of using these variables. The hardcoded values match the required prefixes, so it’s not wrong, but if you intended these to be configurable, you may want to reference the variables here.
| variable "storage_account_name_for_artifacts" { | ||
| type = string | ||
| default = "ands9prostorrage" |
There was a problem hiding this comment.
This default value for storage_account_name_for_artifacts no longer conflicts with the backend requirement because artifacts storage can be any name; just be aware it differs from storage_account_name (the backend account), which is correctly set to "yourstorageaccount". If you want consistency, you could rename it, but it’s not required by the task.
https://github.com/AndS9/devops_todolist_terraform_task/actions/runs/29312444076/job/87018962657