Develop#55
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 there are several critical blockers that will prevent the task from working and from passing the checklist-based grading. The Azure backend is misconfigured: backend.tf uses storage_account_name = "bematestorageaccount" instead of the required yourstorageaccount, and the backend storage account/container must exist before terraform init or initialization will fail ([CHECKLIST ITEM #15/#16/#25]). The network module currently creates a public IP with allocation_method = "Static" instead of the required "Dynamic", which violates the task spec and will fail automated checks ([CHECKLIST ITEM #9/#20]). The naming and values for core resources are not enforced in code or via a committed terraform.tfvars (e.g., resource group mate-azure-task-12, location uksouth, public IP name linuxboxpip, DNS prefix matetask, subnet name default, NSG name defaultnsg, VM name matebox, VM size Standard_B1s, storage container task-artifacts), so without these exact values the grader cannot verify your solution ([CHECKLIST ITEM #20/#22/#24/#27]). The CustomScript extension and install script are also fragile: install-app.sh lacks fail-fast behavior and existence checks for todoapp.service, and the extension’s fileUris/install_app_sh_git_path must point to a raw install-app.sh file rather than a git repo URL, otherwise the script won’t actually run on the VM ([CHECKLIST ITEM #2/#13]).
To move this forward, you should: 1) update backend.tf to use storage_account_name = "yourstorageaccount" and ensure that account + tfstate container exist beforehand; 2) change the public IP to allocation_method = "Dynamic"; 3) either add a terraform.tfvars file or defaults so all required values (RG, location, names, prefixes, VM name/size, container name) match the task exactly; 4) fix the compute module/NIC naming so the NIC is ${var.vm_name}-nic and align the SSH public key variable with the required linuxboxsshkey; and 5) make install-app.sh robust (add set -e, check that the repo and todoapp.service exist, and ensure the VM extension downloads the script via a direct raw URL and executes it as bash ./install-app.sh). Once these are addressed, your modular structure, resource coverage, and outputs should fully satisfy the task, and you’ll be in a good position for the automated checks to pass.
✨ 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
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 there are still several critical blockers that prevent the task from being completed and graded successfully. Most importantly, core requirements are not fully enforced in Terraform: key names and values like resource_group_name = "mate-azure-task-12", location = "uksouth", vm_name = "matebox", vm_size = "Standard_B1s", public IP linuxboxpip, DNS label matetask, NIC name ${vm_name}-nic, and storage container task-artifacts are not guaranteed by defaults/validation or a committed terraform.tfvars, and .gitignore currently ignores *.tfvars, blocking the required terraform.tfvars from being committed. The CustomScript VM extension setup is also not correct: the compute module uses install_app_sh_git_path as if it were a repo URL rather than a raw install-app.sh URL, and install-app.sh itself does not use set -euo pipefail or robust checks on git clone, file existence (e.g. todoapp.service), and directory preparation, which can cause silent failures during provisioning. In addition, several variable names do not match the spec (ssh_public_key vs ssh_key_public, dns_prefix vs dns_label, net_security_group_name vs network_security_group_name) and the NIC name is built from a free-form network_interface_name_sufix, so automated checks looking for exact resource names may fail. Please (1) fix .gitignore so terraform.tfvars can be committed and add or default all checklist-required values, (2) enforce exact resource names/prefixes for vnet/subnet/NSG/public IP/VM/NIC/storage container, (3) change the compute module to take a raw install-app.sh URL and run bash ./install-app.sh, and (4) harden install-app.sh with set -euo pipefail and explicit error checks on clone, file presence, and copy/move operations. Once these are addressed, your structure and module decomposition look solid and should be in good shape for automated grading and a reliable deployment.
✨ 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
| # when running the script as VM extention. | ||
| apt-get update -yq | ||
| apt-get install python3-pip -yq | ||
| apt-get install -yq git |
There was a problem hiding this comment.
This installs git but has no error handling here. Prefer combining package installs (e.g. apt-get install -yq python3-pip git) and/or add set -e at the top so the script fails fast if installation fails.
|
|
||
| # Create a directory for the app and download the files. | ||
| mkdir /app | ||
| # Create a directory for the app and download the files. |
There was a problem hiding this comment.
Creating /app is fine, but the script does not check for errors. Add set -euo pipefail at the top and consider verifying the directory was created and is writable before continuing.
| # Create a directory for the app and download the files. | ||
| mkdir /app | ||
| # Create a directory for the app and download the files. | ||
| mkdir -p /app |
There was a problem hiding this comment.
mkdir -p /app is fine, but you should ensure the script fails on error and consider cleaning or preparing the directory idempotently (e.g. remove stale files if appropriate).
| mkdir -p /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/salabam/devops_todolist_terraform_task.git |
There was a problem hiding this comment.
This hard-coded git clone will break when the VM extension is expected to download a raw install-app.sh. Make the repo path configurable or (better) have the extension supply a raw script URL. Also check git clone exit status and verify the expected devops_todolist_terraform_task/app path exists before cp-ing.
| module "storage_module" { | ||
| source = "./modules/storage" | ||
| storage_account_name = var.storage_account_name | ||
| storage_container_name = var.storage_container_name |
There was a problem hiding this comment.
The storage module is being passed storage_container_name here. The task requires the storage container name to be task-artifacts — ensure this value is set in your committed root variables/terraform.tfvars (or set a default) so the grader can verify it.
| virtual_network_name = var.virtual_network_name | ||
| vnet_address_prefix = var.vnet_address_prefix | ||
| subnet_name = var.subnet_name | ||
| subnet_address_prefix = var.subnet_address_prefix | ||
| net_security_group_name = var.net_security_group_name | ||
| public_ip_name = var.public_ip_name | ||
| dns_prefix = var.dns_prefix |
There was a problem hiding this comment.
You pass many network-related inputs here. The task requires specific names: virtual network vnet, subnet default, network security group defaultnsg, public IP name linuxboxpip, and a DNS label variable named dns_label (prefix matetask + random). Currently your module uses net_security_group_name, public_ip_name and dns_prefix — align these names/values with the spec or document why they differ so automated checks can validate them.
| location = azurerm_resource_group.rg.location | ||
| subnet_id = module.network_module.subnet_id | ||
| public_ip_id = module.network_module.public_ip_id | ||
| network_interface_name_sufix = var.network_interface_name_sufix |
There was a problem hiding this comment.
The compute module receives network_interface_name_sufix and composes the NIC name as ${var.vm_name}${var.network_interface_name_sufix} in the module. The spec requires the NIC name be exactly ${var.vm_name}-nic. Either set network_interface_name_sufix = "-nic" in terraform.tfvars or change the module to build the name explicitly as ${var.vm_name}-nic to enforce the required pattern.
| 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.
ssh_public_key is passed to the compute module here. The task expects the root variable ssh_key_public / SSH key content to be provided and the VM to use the key named linuxboxsshkey per the spec. Make sure your root variable name and value match the spec (or change module variable names accordingly) so grading can verify the SSH configuration.
| vm_size = var.vm_size | ||
| admin_username = var.admin_username | ||
| ssh_public_key = var.ssh_public_key | ||
| install_app_sh_git_path = var.install_app_sh_git_path |
There was a problem hiding this comment.
install_app_sh_git_path is being passed to the compute module and used as the VM extension fileUris. The CustomScript extension must fetch a raw install-app.sh script (not a repo root URL). Commit a reachable raw install-app.sh (or use a storage blob URL) and pass its raw URL here; also make the script robust (set -e) so provisioning fails loudly if something goes wrong. Without a raw script URL the extension will fail to run the installer.
| } | ||
|
|
||
| resource "azurerm_resource_group" "rg" { | ||
| name = var.resource_group_name |
There was a problem hiding this comment.
resource_group_name is taken from a variable here — ensure the root variable (or terraform.tfvars) sets it to the required value mate-azure-task-12. The backend and grader expect that specific resource group name. If you rely on a different value, automated checks will fail.
| @@ -0,0 +1,56 @@ | |||
| resource "azurerm_network_interface" "ni" { | |||
| name = "${var.vm_name}${var.network_interface_name_sufix}" | |||
There was a problem hiding this comment.
NIC name is constructed as ${var.vm_name}${var.network_interface_name_sufix}. The task requires the NIC name be exactly ${var.vm_name}-nic. Either change this line to name = "${var.vm_name}-nic" or ensure the root network_interface_name_sufix variable is set to -nic in a committed terraform.tfvars so the required pattern is enforced. This is required for automated checks to pass.
| admin_ssh_key { | ||
| username = var.admin_username | ||
| public_key = var.ssh_public_key |
There was a problem hiding this comment.
You use var.ssh_public_key for admin_ssh_key.public_key. The task expects the SSH key variable/name and content to follow the spec (SSH key linuxboxsshkey and root variable named ssh_key_public). Align variable names and values between root and module (or rename the module variable) so the grader can verify the SSH configuration.
| fileUris = [ | ||
| var.install_app_sh_git_path |
There was a problem hiding this comment.
The VM extension's fileUris uses var.install_app_sh_git_path. The CustomScript extension needs a raw, directly-downloadable install-app.sh URL (or a blob URL in the storage container) — not a repo root link. Commit/host a raw install-app.sh and pass its raw URL here (or rename var to install_app_sh_url), otherwise the extension will fail to fetch/execute the script.
| fileUris = [ | ||
| var.install_app_sh_git_path | ||
| ], | ||
| commandToExecute = "bash install-app.sh" |
There was a problem hiding this comment.
commandToExecute runs bash install-app.sh. Make sure the referenced install-app.sh exists at the provided URL and that the script is hardened (e.g. set -euo pipefail or at minimum set -e) and idempotent so provisioning fails loudly on error and does not leave the VM in a partially-configured state.
| source_image_reference { | ||
| publisher = "Canonical" | ||
| offer = "0001-com-ubuntu-server-jammy" | ||
| sku = "22_04-lts" | ||
| version = "latest" |
There was a problem hiding this comment.
The VM image and SKU here use Ubuntu 22.04 (Canonical jammy, sku 22_04-lts) which matches the requirement for Ubuntu2204 — keep this but ensure var.vm_size and var.vm_name provided at root match the required Standard_B1s and matebox values. The module relies on root inputs rather than enforcing defaults here.
| @@ -0,0 +1,14 @@ | |||
| resource "azurerm_storage_account" "st" { | |||
| name = var.storage_account_name | |||
There was a problem hiding this comment.
name = var.storage_account_name — ensure the value provided in terraform.tfvars meets Azure storage account naming constraints (lowercase letters and numbers, 3–24 characters) and is globally unique. Also note the azurerm backend requires the backend storage account/container to exist before terraform init; creating a storage account in this same Terraform configuration will not satisfy that requirement for the backend.
| } | ||
|
|
||
| resource "azurerm_storage_container" "cnt" { | ||
| name = var.storage_container_name |
There was a problem hiding this comment.
name = var.storage_container_name — the task requires the storage container be named task-artifacts. Please ensure terraform.tfvars sets storage_container_name = "task-artifacts" (or change this line to enforce that name) so the grader can validate the requirement.
| variable "storage_container_name" { | ||
| description = "The name of the storage container" | ||
| type = string |
There was a problem hiding this comment.
The module expects storage_container_name — the task requires this container be named task-artifacts. Add a default here (e.g. default = "task-artifacts") or ensure your root terraform.tfvars sets storage_container_name = "task-artifacts" so the grader can verify it.
| variable "storage_account_name" { | ||
| description = "The name of the storage account" | ||
| type = string |
There was a problem hiding this comment.
storage_account_name is declared here; confirm the value you pass from the root matches any backend/storage usage (the backend expects yourstorageaccount for remote state per the spec) or document if you manage backend storage separately. Ensure the account/container exist before terraform init.
| @@ -0,0 +1,86 @@ | |||
| ### I made sure that all variables are in terraform.tfvars | |||
There was a problem hiding this comment.
Top comment says variables are in terraform.tfvars. Make sure you actually commit a terraform.tfvars file with the exact required values (see task spec) because the modules rely on caller-provided values and the grader expects specific names/values.
| ### I made sure that all variables are in terraform.tfvars | ||
| ### I don't need you to write "check it!" to every variable. | ||
|
|
||
| variable "resource_group_name" { |
There was a problem hiding this comment.
The resource_group_name variable must be set to the exact value mate-azure-task-12 per the checklist. Add that value in a committed terraform.tfvars or set a default here so the grader sees it.
| type = string | ||
| } | ||
|
|
||
| variable "location" { |
There was a problem hiding this comment.
The location variable must be uksouth according to the task. Provide that in terraform.tfvars or set a default to enforce it.
| type = string | ||
| } | ||
|
|
||
| variable "storage_account_name" { |
There was a problem hiding this comment.
The backend requires a storage account named yourstorageaccount (backend.tf). If you intend this module to create the backend storage account, set storage_account_name to yourstorageaccount or document that this account/container must be created before terraform init.
| type = string | ||
| } | ||
|
|
||
| variable "storage_container_name" { |
There was a problem hiding this comment.
The storage container used by the task must be task-artifacts. Enforce this by adding storage_container_name = "task-artifacts" in a committed tfvars or set a default here.
| type = string | ||
| } | ||
|
|
||
| variable "virtual_network_name" { |
There was a problem hiding this comment.
The task requires the virtual network name vnet. Ensure virtual_network_name is set to vnet in your tfvars (or set it as a default) so the module matches the specification.
| type = string | ||
| } | ||
|
|
||
| variable "vnet_address_prefix" { |
There was a problem hiding this comment.
vnet_address_prefix is a list — that is fine, but the checklist expects 10.0.0.0/16. Provide vnet_address_prefix = ["10.0.0.0/16"] in your tfvars so the network matches the requirement.
| type = list(string) | ||
| } | ||
|
|
||
| variable "subnet_name" { |
There was a problem hiding this comment.
The subnet name must be default and its prefix must be 10.0.0.0/24. Make sure subnet_name and subnet_address_prefix are set to those exact values in a committed tfvars file.
| type = list(string) | ||
| } | ||
|
|
||
| variable "public_ip_name" { |
There was a problem hiding this comment.
The variable here is public_ip_name but the task specification names it public_ip_address_name. The grader may expect the latter — either rename the variable to public_ip_address_name or ensure tfvars sets public_ip_name = "linuxboxpip" (and commit tfvars).
| type = string | ||
| } | ||
|
|
||
| variable "dns_prefix" { |
There was a problem hiding this comment.
You use dns_prefix here but the spec uses dns_label (the required value is matetask). Either rename the variable to dns_label or ensure dns_prefix = "matetask" is present in tfvars; the DNS prefix must be exactly matetask (random suffix is appended elsewhere).
| type = string | ||
| } | ||
|
|
||
| variable "net_security_group_name" { |
There was a problem hiding this comment.
The variable net_security_group_name doesn't match the spec name network_security_group_name. Also the NSG must be named defaultnsg. Rename or ensure tfvars sets net_security_group_name = "defaultnsg" so the resource name matches the checklist.
| type = string | ||
| } | ||
|
|
||
| variable "vm_name" { |
There was a problem hiding this comment.
The vm_name must be matebox and vm_size must be Standard_B1s. Ensure these exact values are set in terraform.tfvars (or set defaults here) to satisfy the grader.
| type = string | ||
| } | ||
|
|
||
| variable "network_interface_name_sufix" { |
There was a problem hiding this comment.
Using a network_interface_name_sufix that is concatenated with vm_name is OK only if the suffix equals -nic. The task requires the NIC name to be ${var.vm_name}-nic. Either change the module to derive "${var.vm_name}-nic" or ensure the committed tfvars set network_interface_name_sufix = "-nic" (note also the spelling 'sufix' — consider renaming to 'suffix' for clarity).
| type = string | ||
| } | ||
|
|
||
| variable "ssh_public_key" { |
There was a problem hiding this comment.
The task expects the SSH public key variable named ssh_key_public (with the content your-public-key-content / or the grader's linuxboxsshkey). Here it's ssh_public_key — either rename to ssh_key_public or ensure tfvars provides the required key content and documents that it corresponds to linuxboxsshkey.
| type = string | ||
| } | ||
|
|
||
| variable "install_app_sh_git_path" { |
There was a problem hiding this comment.
The compute module's install script variable install_app_sh_git_path is present. Important: the VM extension expects a raw file URL (e.g. a raw GitHub URL to install-app.sh), not a repository URL. Make sure the tfvars value points to the raw script URL so the CustomScript extension can download and execute it.
| *.tfstate | ||
| *.tfstate.* | ||
|
|
||
| *.tfvars |
There was a problem hiding this comment.
*.tfvars will ignore the required terraform.tfvars file which the task requires in the repository (Main Configuration and Variables). This will prevent graders / CI from seeing the enforced variable values. Remove this line or add an explicit allow rule such as !terraform.tfvars so the root terraform.tfvars can be committed.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "yourstorageaccount" |
There was a problem hiding this comment.
storage_account_name = "yourstorageaccount" is correct per the task, but the storage account and the tfstate container must exist before running terraform init — otherwise terraform init will fail (see checklist items about backend pre-creation). Create them manually or ensure your storage module creates them and that they exist prior to init.
| storage_account_name = "yourstorageaccount" | ||
| container_name = "tfstate" | ||
| key = "terraform.tfstate" | ||
| use_oidc = true |
There was a problem hiding this comment.
use_oidc = true enables OIDC auth for the backend which requires compatible Terraform/CI and provider support (Terraform v1.6+ and Azure provider configuration). If your environment/CI runner does not support OIDC, remove or adjust this setting.
| variable "resource_group_name" { | ||
| description = "The name of the resource group" | ||
| type = string |
There was a problem hiding this comment.
The task requires the resource group to be mate-azure-task-12 (must be enforced in code or via committed tfvars). Either set a default = "mate-azure-task-12" here or add a validation to ensure the value matches the required name so automated checks can verify it.
| variable "location" { | ||
| description = "The location for all resources." | ||
| type = string |
There was a problem hiding this comment.
The task requires location to be uksouth. Add a default = "uksouth" or a validation block so the module cannot be instantiated with a different location (automated grader expects this exact value).
| variable "vm_name" { | ||
| description = "The name of the virtual machine" | ||
| type = string |
There was a problem hiding this comment.
The VM must be named matebox per the specification. Set default = "matebox" or add a validation to enforce this value (automated checks require the exact name).
| variable "vm_size" { | ||
| description = "The size of the virtual machine" | ||
| type = string |
There was a problem hiding this comment.
The VM size must be Standard_B1s according to the task. Provide default = "Standard_B1s" or validation here to enforce the required size.
| variable "network_interface_name_sufix" { | ||
| description = "The sufix of the network interface, will be concatenated with vm_name" | ||
| type = string |
There was a problem hiding this comment.
network_interface_name_sufix is misspelled (sufix) and relying on a free-form suffix can break the requirement that the NIC name is exactly ${var.vm_name}-nic. Remove this variable or default it to -nic, and build the NIC name in the module as "${var.vm_name}-nic" to satisfy the checklist.
| variable "ssh_public_key" { | ||
| description = "The path to the ssh key for sign in to system" | ||
| type = string |
There was a problem hiding this comment.
The variable ssh_public_key name and description are problematic: the task expects a public key content variable (root variable named ssh_key_public / required linuxboxsshkey). Rename or map this to the expected name and update the description to clarify it should contain the public key content (not a filesystem path).
| variable "install_app_sh_git_path" { | ||
| description = "The path to the git repo with install script" | ||
| type = string |
There was a problem hiding this comment.
install_app_sh_git_path is described as a git repo path. The CustomScript extension must download a raw install-app.sh file (raw URL) and execute it as bash ./install-app.sh. Rename this variable to reflect a raw script URL (e.g. install_app_sh_url) and ensure the module uses that raw URL in the VM extension fileUris and executes bash ./install-app.sh.
| variable "vm_name" { | ||
| description = "The name of the virtual machine" | ||
| type = string |
There was a problem hiding this comment.
This module does not expose or default the VM image. The task requires the VM image be Ubuntu2204. Add a variable (or hardcode in the module) to ensure the image is set to Canonical:0001-com-ubuntu-server-jammy:22_04-lts:latest (or an equivalent Ubuntu2204 reference), or document that the module will always use Ubuntu2204.
| variable "resource_group_name" { | ||
| description = "The name of the resource group" | ||
| type = string |
There was a problem hiding this comment.
The task requires the resource group be exactly mate-azure-task-12 (checklist). Add a default = "mate-azure-task-12" or a validation to enforce that value so the grader can verify it.
| variable "location" { | ||
| description = "The location for all resources." | ||
| type = string |
There was a problem hiding this comment.
The task requires location to be uksouth. Add default = "uksouth" or a validation block to prevent other values (automated checks expect this exact location).
| variable "virtual_network_name" { | ||
| description = "The name of the virtual network" | ||
| type = string |
There was a problem hiding this comment.
The virtual network must be named vnet per the spec. Set default = "vnet" or validate the value to guarantee the required name.
| variable "vnet_address_prefix" { | ||
| description = "The list of prefixes" | ||
| type = list(string) |
There was a problem hiding this comment.
vnet_address_prefix is declared as list(string), but the task specifies a single prefix 10.0.0.0/16. Change the type to string and add default = "10.0.0.0/16" (or add validation) so the module uses the exact prefix expected by the grader.
| variable "subnet_name" { | ||
| description = "The name of the sub network" | ||
| type = string |
There was a problem hiding this comment.
The subnet must be named default. Add default = "default" or a validation to ensure the subnet name matches the checklist requirement.
| variable "subnet_address_prefix" { | ||
| description = "The list of prefixes" | ||
| type = list(string) |
There was a problem hiding this comment.
subnet_address_prefix is a list(string) but the task requires a single prefix 10.0.0.0/24. Change the type to string and add default = "10.0.0.0/24" (or validate) to match the specification.
| variable "public_ip_name" { | ||
| description = "The name of the public ip address" | ||
| type = string |
There was a problem hiding this comment.
The public IP must be named linuxboxpip per the task. Add default = "linuxboxpip" or a validation so the public IP name cannot differ (automated checks look for this exact name).
| variable "dns_prefix" { | ||
| description = "The prefix for the dns label, will be concatenated with random number between 1000 and 9999" | ||
| type = string |
There was a problem hiding this comment.
The DNS prefix must be matetask (you can append a random number in module logic). Add default = "matetask" so the grader can verify the expected prefix is used before randomization.
| variable "net_security_group_name" { | ||
| description = "The name of the network security group" | ||
| type = string |
There was a problem hiding this comment.
The network security group name must be defaultnsg. Add default = "defaultnsg" or validate the value to ensure compliance with the checklist.
| output "public_ip_fqdn" { | ||
| value = azurerm_public_ip.pip.fqdn |
There was a problem hiding this comment.
This output uses azurerm_public_ip.pip.fqdn. Ensure the public IP resource in this module is actually declared as azurerm_public_ip "pip" { ... }. Also note fqdn will only be populated when a dns_label is set on the public IP (the task requires a matetask-prefixed DNS label).
| output "subnet_id" { | ||
| value = azurerm_subnet.subnet.id |
There was a problem hiding this comment.
This output references azurerm_subnet.subnet.id — confirm the subnet resource is named subnet in the module (resource name must match). The task requires the subnet be named default at the Azure level; make sure the resource uses the correct Azure name argument while the Terraform resource name here can remain subnet.
| output "public_ip_id" { | ||
| value = azurerm_public_ip.pip.id |
There was a problem hiding this comment.
This references azurerm_public_ip.pip.id. As with the first comment, confirm the public IP resource in the module is declared with the local name pip, otherwise Terraform will fail during plan/apply.
| output "public_ip_address" { | ||
| value = azurerm_public_ip.pip.ip_address |
There was a problem hiding this comment.
azurerm_public_ip.pip.ip_address will only be available after the public IP is created. That's expected, but keep in mind if you rely on this output in other resources during the same apply you may need to manage dependencies. Consider adding description fields to outputs to improve clarity.
| resource "azurerm_virtual_network" "vnet" { | ||
| name = var.virtual_network_name | ||
| address_space = var.vnet_address_prefix | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location |
There was a problem hiding this comment.
address_space typically expects a list of CIDR strings. If var.vnet_address_prefix is a single string, wrap it as a list here (e.g. address_space = [var.vnet_address_prefix]) or change the variable to be a list. Also the task requires the vnet be named vnet — enforce this via a default or validation on the variable so automated checks can verify it.
| resource "azurerm_subnet" "subnet" { | ||
| name = var.subnet_name | ||
| address_prefixes = var.subnet_address_prefix | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name |
There was a problem hiding this comment.
address_prefixes on azurerm_subnet expects a list. If var.subnet_address_prefix is a string, use address_prefixes = [var.subnet_address_prefix] or change the variable type accordingly. Also the subnet name must be default per the task — ensure the variable enforces that value (default/validation) so graders can detect it.
| allocation_method = "Dynamic" | ||
| sku = "Standard" |
There was a problem hiding this comment.
You set allocation_method = "Dynamic" (correct per task) but sku = "Standard" on line 25. In Azure, Standard public IPs are typically static; combining Standard SKU with Dynamic can be invalid. Use sku = "Basic" (or omit sku) if you need Dynamic allocation. Also ensure the public IP name is linuxboxpip (enforce via variable default/validation).
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| sku = "Standard" | ||
| domain_name_label = "${var.dns_prefix}${random_integer.suffix.result}" |
There was a problem hiding this comment.
The domain name label is constructed with var.dns_prefix but the task expects a dns_label/matetask variable. Ensure the variable name matches the root variable (or map it) and that the value is matetask (you may append the random suffix). Otherwise the grader will not find the expected DNS prefix.
| resource "azurerm_network_security_group" "nsg" { | ||
| name = var.net_security_group_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name |
There was a problem hiding this comment.
azurerm_network_security_group.nsg must have the name defaultnsg per the requirements. Ensure var.net_security_group_name has a default or validation to enforce defaultnsg so automated checks pass.
| dynamic "security_rule" { | ||
| for_each = local.security_rules |
There was a problem hiding this comment.
The dynamic block uses for_each = local.security_rules. Make sure local.security_rules is defined in this module (locals.tf) or passed in; if it's undefined Terraform will fail. If you don't need custom rules, set local.security_rules = [] to avoid errors.
| output "storage_account_id" { | ||
| value = azurerm_storage_account.st.id |
There was a problem hiding this comment.
These outputs assume resources are declared as azurerm_storage_account.st and azurerm_storage_container.cnt. If the storage resources in this module use different local names, Terraform will fail with an undefined resource error. Confirm the resource block names in main.tf or update these references to match.
| output "storage_container_id" { | ||
| value = azurerm_storage_container.cnt.id |
There was a problem hiding this comment.
The task specification requires the storage container be named task-artifacts. Ensure the storage container resource in this module actually sets name = "task-artifacts". It would also be helpful to export the container name (not just its id) so other modules or the backend configuration can reference the exact name.
| output "public_ip_address" { | ||
| value = module.network_module.public_ip_address |
There was a problem hiding this comment.
This output references module.network_module.public_ip_address. Make sure the network module (modules/network/outputs.tf) defines an output named public_ip_address that returns the IP address (e.g. the azurerm_public_ip.pip.ip_address). If the module block in root main.tf uses a different name (for example module.network), update this reference to match the module block name.
| output "public_ip_fqdn" { | ||
| value = module.network_module.public_ip_fqdn |
There was a problem hiding this comment.
This output references module.network_module.public_ip_fqdn. Confirm the network module actually exports public_ip_fqdn (for example returning the azurerm_public_ip.pip.fqdn). If you intend to expose the DNS name ensure the public IP resource sets a domain_name_label and the module output returns the fqdn. Also confirm the root module block is named network_module.
| output "vm_id" { | ||
| value = module.compute_module.vm_id |
There was a problem hiding this comment.
This output references module.compute_module.vm_id. Ensure the compute module (modules/compute/outputs.tf) exports an output named vm_id (for example the azurerm_linux_virtual_machine.matebox.id) and that the root main.tf module block is named compute_module. If names differ Terraform will report an undefined attribute error.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
3 participants
No description provided.