experimental-1#36
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your work shows a solid structure and well-implemented network and storage modules, but there are critical blockers that prevent successful deployment and violate explicit checklist items.
Critical issues to fix (blockers):
- backend.tf: Line with
storage_account_name = "leidenstorage"must be"yourstorageaccount"to satisfy checklist items 1.5 and 2.8. This is a literal requirement and must match exactly. - modules/compute/main.tf: The file instantiates the
computemodule instead of defining resources. In a module’smain.tf, define the resources themselves (e.g.,azurerm_network_interface,azurerm_linux_virtual_machine,azurerm_virtual_machine_extension). This current setup will error and no VM/app will be created. - modules/compute/variables.tf: Variables are network-related and don’t match the required compute interface. Replace with only compute-relevant inputs:
vm_name,vm_size,ssh_key_public,subnet_id,public_ip_id,nsg_id,script_url. - modules/compute/outputs.tf: Outputs reference
azurerm_linux_virtual_machine.mainandazurerm_network_interface.mainwhich don’t exist yet. After you define these resources inmain.tf, update outputs accordingly.
Important minor fix:
- modules/network/variables.tf: Change types of
vnet_address_prefixandsubnet_address_prefixtolist(string)to match AzureRM usage.
How to proceed (hints):
- In compute/main.tf, create a NIC attached to
var.subnet_id, associate NSG/Public IP as required, create an Ubuntu 22.04 VM (image: Ubuntu2204, sizeStandard_B1s) with SSH key auth, and add a Custom Script extension to runinstall-app.shfromvar.script_url. - Ensure the storage container name used for app artifacts is
task-artifacts, and that the DNS label ismatetaskplus a random integer (already correct in network).
You’re very close—network and storage look good. Once the compute module is properly implemented and the backend name corrected, you should meet the requirements. Keep going!
✨ 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
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "leidenstorage" |
There was a problem hiding this comment.
The storage_account_name must be set to yourstorageaccount as specified in the requirements (checklist item 1.5 and 2.8). Using leidenstorage does not meet the literal value requirement. Please update this value to match the task description exactly.
| @@ -0,0 +1,12 @@ | |||
| module "compute" { | |||
There was a problem hiding this comment.
This is not a valid module implementation. In a module's main.tf, you should define resources (e.g., VM, NIC, extension), not instantiate the module itself. This will cause errors and does not meet the requirements. Replace this with the actual resource definitions for the compute module.
| @@ -0,0 +1,9 @@ | |||
| output "vm_id" { | |||
| value = azurerm_linux_virtual_machine.main.id | |||
There was a problem hiding this comment.
The referenced resources (azurerm_linux_virtual_machine.main and azurerm_network_interface.main) do not exist in this module. You must define these resources in main.tf for these outputs to work.
| } | ||
|
|
||
| output "vm_private_ip" { | ||
| value = azurerm_network_interface.main.private_ip_address |
There was a problem hiding this comment.
The referenced resource (azurerm_network_interface.main) does not exist in this module. You must define this resource in main.tf for this output to work.
| variable "resource_group_name" { | ||
| description = "Name of the resource group" | ||
| type = string | ||
| } | ||
| variable "location" { | ||
| description = "Azure region for resources" | ||
| type = string | ||
| } | ||
| variable "virtual_network_name" { | ||
| description = "Name of the Virtual Network" | ||
| type = string | ||
| } | ||
| variable "vnet_address_prefix" { | ||
| description = "Address prefix for the Virtual Network" | ||
| type = list(string) # ЗМІНЕНО: тепер list(string) | ||
| } | ||
| variable "subnet_name" { | ||
| description = "Name of the Subnet" | ||
| type = string | ||
| } | ||
| variable "subnet_address_prefix" { | ||
| description = "Address prefix for the Subnet" | ||
| type = list(string) # ЗМІНЕНО: тепер list(string) | ||
| } | ||
| variable "network_security_group_name" { | ||
| description = "Name of the Network Security Group" | ||
| type = string | ||
| } | ||
| variable "public_ip_address_name" { | ||
| description = "Name of the Public IP Address" | ||
| type = string | ||
| } | ||
| variable "dns_label" { | ||
| description = "DNS label for the Public IP Address" | ||
| type = string | ||
| } No newline at end of file |
There was a problem hiding this comment.
These variables are network-related and should not be defined in the compute module. The compute module should only define variables relevant to compute resources (e.g., vm_name, vm_size, ssh_key_public, subnet_id, public_ip_id, nsg_id, script_url). Update this file to match the required interface.
|
|
||
| variable "vnet_address_prefix" { | ||
| description = "Префікс адреси для віртуальної мережі." | ||
| type = string |
There was a problem hiding this comment.
The type for vnet_address_prefix should be list(string) to match its usage in the main.tf and AzureRM resource requirements. Update this type to avoid errors.
|
|
||
| variable "subnet_address_prefix" { | ||
| description = "Префікс адреси для підмережі." | ||
| type = string |
There was a problem hiding this comment.
The type for subnet_address_prefix should be list(string) to match its usage in the main.tf and AzureRM resource requirements. Update this type to avoid errors.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because several critical blockers prevent a successful deploy and violate explicit checklist requirements. Key issues to fix:
- modules/network/outputs.tf: Missing
public_ip_idoutput (required by root/compute wiring). Without this, the NIC cannot associate to the Public IP [HIGH]. Addoutput "public_ip_id" { value = azurerm_public_ip.pip.id }. - modules/storage/outputs.tf and modules/storage/main.tf: No
script_urloutput and no blob forinstall-app.sh. The compute module depends onmodule.storage.script_urlto run the Custom Script extension [HIGH]. Upload/create the blob and output its URL (public or SAS). - modules/compute/main.tf: NIC is not associated with the NSG [HIGH]. Add
network_security_group_id = var.nsg_id(or an association resource) and ensurevariable "nsg_id" { type = string }exists in modules/compute/variables.tf. - modules/compute/main.tf: Custom Script extension
settingsis incorrect [HIGH]. UsefileUrisandcommandToExecuteso it downloadsinstall-app.shfromvar.script_urland executes it. - .terraform/modules/modules.json shows a duplicate/empty entry for compute [MED/HIGH]. Check that the compute module is not instantiating itself and re-run
terraform initto regenerate.
Minor but important: ensure the SSH key naming requirement for linuxboxsshkey is met (e.g., create azurerm_ssh_public_key.linuxboxsshkey) and fix outputs in modules/compute/outputs.tf to reference actual resource names and correct NIC private IP attribute path. Also run terraform fmt -recursive to fix the CI formatting failure. You’ve got a solid structure in place—addressing the above will unblock deployment and 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 @@ | |||
| {"Modules":[{"Key":"","Source":"","Dir":"."},{"Key":"compute","Source":"./modules/compute","Dir":"modules/compute"},{"Key":"compute.compute","Source":"./modules/compute","Dir":""},{"Key":"network","Source":"./modules/network","Dir":"modules/network"},{"Key":"storage","Source":"./modules/storage","Dir":"modules/storage"}]} No newline at end of file | |||
There was a problem hiding this comment.
This modules.json line contains an empty top-level module entry and a duplicate entry compute.compute with Dir set to an empty string. That pattern commonly appears when a module is (accidentally) instantiated inside itself or when module source/path references are inconsistent.
- Check
modules/compute/main.tffor anymodule "compute" { source = "./modules/compute" }(or similar) and remove such self-instantiation — per the task checklist the compute module must define the NIC/VM/extension resources directly, not instantiate itself. - Verify the root
main.tfusesmodule "compute" { source = "./modules/compute" }only in the root, and that module paths are correct. - After fixing, run
terraform initto regenerate.terraform/modules/modules.jsonand confirm there are no duplicate/empty entries.
Fixing this addresses the high-priority issue where the compute module was used incorrectly and may block resource creation.
| name = "${var.vm_name}-nic" | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
|
|
There was a problem hiding this comment.
The NIC resource currently does not associate the Network Security Group. The checklist requires the NIC to be associated with the provided NSG. Add an NSG association (for example by adding network_security_group_id = var.nsg_id or using the appropriate association resource) and ensure var.nsg_id is provided to the module. This prevents the NIC from being protected/filtered by the required defaultnsg.
| admin_ssh_key { | ||
| username = "adminuser" | ||
| public_key = var.ssh_key_public |
There was a problem hiding this comment.
The admin_ssh_key block uses var.ssh_key_public, which is fine to set the VM SSH key content, but the task explicitly requires an SSH key named linuxboxsshkey. If the checklist expects an azurerm_ssh_public_key resource named linuxboxsshkey, create that resource (e.g., resource "azurerm_ssh_public_key" "linuxboxsshkey" { ... }) or otherwise document how the SSH key name requirement is satisfied. Right now there is no resource named linuxboxsshkey in this module.
| settings = <<SETTINGS | ||
| { | ||
| "script": "${var.script_url}" | ||
| } | ||
| SETTINGS |
There was a problem hiding this comment.
The Custom Script extension settings block is not using the format required to download and execute install-app.sh. "script": "${var.script_url}" is not a valid settings payload for the Linux CustomScript extension. Use a JSON payload with fileUris (list of URLs) and commandToExecute. Example:
settings = <<SETTINGS
{
"fileUris": ["${var.script_url}"],
"commandToExecute": "bash install-app.sh"
}
SETTINGS
Ensure var.script_url points to the install-app.sh location and that the command executes it so the ToDo app is installed as required by the checklist.
| source_image_reference { | ||
| publisher = "Canonical" | ||
| offer = "0001-com-ubuntu-server-jammy" | ||
| sku = "22_04-lts" | ||
| version = "latest" | ||
| } |
There was a problem hiding this comment.
Double-check that the source_image_reference matches the explicit task requirement Ubuntu2204. The current values (publisher = "Canonical", offer = "0001-com-ubuntu-server-jammy", sku = "22_04-lts") will provision Ubuntu 22.04 and are acceptable, but confirm the root-level variable vm_size is set to Standard_B1s (that value is used from var.vm_size).
| } | ||
|
|
||
| resource "azurerm_resource_group" "rg" { | ||
| name = var.resource_group_name |
There was a problem hiding this comment.
The resource group is created from var.resource_group_name. Per the task checklist the resource group must be named mate-azure-task-12 (or that exact literal must be the default in the root variables). Ensure variables.tf sets resource_group_name = "mate-azure-task-12" so this resource matches the checklist (checklist: resource group name requirement).
| @@ -0,0 +1,51 @@ | |||
| terraform { | |||
There was a problem hiding this comment.
The terraform {} block is present here but the task requires a remote backend configured in backend.tf with storage_account_name = "yourstorageaccount" (literal), container tfstate, resource group mate-azure-task-12, and key terraform.tfstate. Make sure you have a separate backend.tf file with those exact values. If you have backend code elsewhere, confirm storage_account_name is exactly yourstorageaccount (this is a high-priority literal requirement).
| subnet_address_prefix = var.subnet_address_prefix | ||
| 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.
You pass dns_label = var.dns_label into the network module. The checklist requires the DNS label to start with the literal matetask and include a random number appended. Verify the network module appends a random integer (or otherwise constructs matetask<random>) when creating the Public IP's dns_name/dns_label so the final label meets the requirement.
| resource_group_name = azurerm_resource_group.rg.name | ||
| location = azurerm_resource_group.rg.location | ||
| storage_account_name = var.storage_account_name_app_artifacts | ||
| storage_container_name = var.storage_container_name_app_artifacts |
There was a problem hiding this comment.
The storage module is passed storage_container_name = var.storage_container_name_app_artifacts. The task requires the storage container name to be exactly task-artifacts. Ensure var.storage_container_name_app_artifacts default (or the value in terraform.tfvars) is exactly task-artifacts so the created container matches the checklist.
| vm_size = var.vm_size | ||
| ssh_key_public = var.ssh_key_public | ||
| subnet_id = module.network.subnet_id | ||
| public_ip_id = module.network.public_ip_id |
There was a problem hiding this comment.
Compute module expects subnet_id, public_ip_id, and nsg_id from the network module (you pass module.network.subnet_id, module.network.public_ip_id, module.network.nsg_id). Confirm modules/network/outputs.tf exports outputs with these exact names (subnet_id, public_ip_id, nsg_id) and that they refer to the Terraform resources used by compute (otherwise the compute module will fail to attach the NIC, NSG or Public IP).
| ssh_key_public = var.ssh_key_public | ||
| subnet_id = module.network.subnet_id | ||
| public_ip_id = module.network.public_ip_id | ||
| script_url = module.storage.script_url |
There was a problem hiding this comment.
You pass script_url = module.storage.script_url into compute. The compute module will use this for the Custom Script extension that must execute install-app.sh. Verify modules/storage/outputs.tf defines script_url and that it points to the install-app.sh location (e.g., a blob URL in the storage account or another reachable URL). The compute module must use that URL in the VM extension so install-app.sh actually runs on provisioning (checklist: CustomScript must run install-app.sh).
| output "vm_id" { | ||
| value = azurerm_linux_virtual_machine.main.id | ||
| description = "The ID of the virtual machine." |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.main.id. The compute module must actually define an azurerm_linux_virtual_machine resource with the resource name main for this to work. If your VM resource in modules/compute/main.tf is named differently (for example azurerm_linux_virtual_machine.matebox), update this output to reference that name. Per the checklist, outputs must reference real resources defined in the compute module.
| output "vm_private_ip" { | ||
| value = azurerm_network_interface.main.private_ip_address | ||
| description = "The private IP address of the virtual machine." |
There was a problem hiding this comment.
This output references azurerm_network_interface.main.private_ip_address. Two issues to address:
- Ensure the compute module defines an
azurerm_network_interfaceresource namedmain(or change this output to the actual resource name used). - The network interface's private IP is typically available under
ip_configuration[0].private_ip_address(e.g.,azurerm_network_interface.main.ip_configuration[0].private_ip_address). Referencing.private_ip_addressdirectly may not be valid with the AzureRM provider. Update the attribute path accordingly so the output resolves to the NIC private IP.
| output "vm_private_ip" { | ||
| value = azurerm_network_interface.main.private_ip_address | ||
| description = "The private IP address of the virtual machine." | ||
| } No newline at end of file |
There was a problem hiding this comment.
Optional suggestion: it is helpful for verification (the ToDo app must be reachable) to also output the VM public IP or public DNS name from the compute module. Consider adding an output like vm_public_ip or vm_fqdn (make sure the compute module obtains the public IP/DNS value) so testers can quickly open the app in a browser.
| name = var.virtual_network_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| address_space = var.vnet_address_prefix |
There was a problem hiding this comment.
The azurerm_virtual_network address_space attribute expects a list of CIDR strings (list(string)). Ensure var.vnet_address_prefix is declared as list(string) and that you pass a list like ["10.0.0.0/16"] so Terraform/azurerm accepts it. This ties to the checklist requirement that the VNet uses 10.0.0.0/16.
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefix |
There was a problem hiding this comment.
The azurerm_subnet address_prefixes attribute expects a list of CIDR strings. Make sure var.subnet_address_prefix is list(string) and contains ["10.0.0.0/24"] to satisfy the checklist requirement that the subnet uses 10.0.0.0/24.
| } | ||
|
|
||
| resource "azurerm_network_security_group" "nsg" { | ||
| name = var.network_security_group_name |
There was a problem hiding this comment.
The NSG name is taken from var.network_security_group_name. The checklist requires the NSG name to be the literal defaultnsg. Verify the variable default (in the module or root variables) is set to defaultnsg, otherwise the created name will not meet the requirement.
| } | ||
|
|
||
| resource "azurerm_public_ip" "pip" { | ||
| name = var.public_ip_address_name |
There was a problem hiding this comment.
Public IP name is set from var.public_ip_address_name. The task requires the Public IP name be linuxboxpip (literal). Ensure the variable default equals linuxboxpip so the resource name matches the checklist.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| domain_name_label = "${var.dns_label}${random_integer.r.result}" |
There was a problem hiding this comment.
You correctly set allocation_method = "Dynamic" and you construct domain_name_label by concatenating var.dns_label with a random integer. The checklist requires the DNS label to start with the literal matetask and include a random number. Make sure var.dns_label default is matetask (literal). Also validate the final label meets Azure DNS naming rules (lowercase, length 3-63).
| tags = { | ||
| environment = "dev" | ||
| } | ||
| } |
There was a problem hiding this comment.
The root and compute module expect outputs like subnet_id, public_ip_id, and nsg_id from this module. Confirm modules/network/outputs.tf exports those exact output names and that they reference azurerm_subnet.subnet.id, azurerm_public_ip.pip.id, and azurerm_network_security_group.nsg.id respectively — otherwise the compute module will not receive the IDs it needs (checklist: wiring between modules).
| } | ||
| } | ||
|
|
||
| resource "random_integer" "r" { |
There was a problem hiding this comment.
You created random_integer.r to append a suffix. That is fine — just confirm the min/max values produce a suffix length that keeps domain_name_label within Azure limits when combined with matetask and any other characters.
| variable "resource_group_name" { | ||
| description = "Name of the resource group" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The variable resource_group_name (used to name the RG) is declared here. The compute module should ideally expose only compute-specific inputs. Per the checklist the compute module should accept vm_name, vm_size, ssh_key_public, subnet_id, public_ip_id, nsg_id, and script_url. Remove resource_group_name from this module unless you have a concrete reason to keep it (or document why). See checklist: modules/compute/variables.tf should expose only compute-relevant inputs.
| variable "location" { | ||
| description = "Azure region for resources" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The location variable is declared here. Location is usually provided at the root and passed into modules only when strictly required. The checklist expects the compute module to focus on compute inputs; consider removing location from the module interface and sourcing it from the root unless you need it inside the module for resource creation (if you keep it, that's fine but explain why).
| variable "ssh_key_public" { | ||
| description = "Public SSH key for the virtual machine" | ||
| type = string | ||
| sensitive = true | ||
| } |
There was a problem hiding this comment.
ssh_key_public is declared and marked sensitive = true. It's fine to mark secrets as sensitive, although a public SSH key normally isn't secret. More importantly, ensure the compute module uses this value when configuring the VM SSH key and that the VM's SSH key resource/name matches the checklist requirement (the task expects the SSH key used to authenticate the VM to be named linuxboxsshkey on the VM or at least referenced accordingly in your VM resource).
| variable "subnet_id" { | ||
| description = "ID of the subnet to connect the VM to" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
subnet_id is present and typed as string — this is correct. Ensure the compute module uses this to create the NIC named ${var.vm_name}-nic attached to this subnet (checklist requires that NIC name).
| variable "public_ip_id" { | ||
| description = "ID of the public IP address to associate with the VM" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
public_ip_id is present and typed as string — this is correct. Make sure the compute module associates the NIC (or VM) with this Public IP so the VM is reachable externally (checklist: VM must be accessible via public IP/DNS).
| variable "script_url" { | ||
| description = "URL of the script to execute on the VM" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The module accepts script_url which is required so the Custom Script Extension can download and run install-app.sh. Confirm that the storage module provides a script_url that points specifically to the install-app.sh blob, and that the compute module uses it in the VM extension to execute the script on provisioning (checklist: VM Extension must execute install-app.sh).
| variable "script_url" { | ||
| description = "URL of the script to execute on the VM" | ||
| type = string | ||
| } |
There was a problem hiding this comment.
Missing required variable: add variable "nsg_id" { description = "ID of the Network Security Group to associate with the NIC" type = string }. The compute module must accept nsg_id so it can associate the NIC with the defaultnsg NSG created by the network module (this was flagged as a HIGH-priority checklist item). Add this variable (e.g., after public_ip_id or script_url) and ensure the module uses it when creating the NIC.
| output "public_ip_address" { | ||
| description = "The Public IP Address of the Linux VM" | ||
| value = azurerm_public_ip.pip.ip_address |
There was a problem hiding this comment.
The compute/root configuration expects module.network.public_ip_id (resource id) but this file only exports the IP address and FQDN. Add an output named public_ip_id with the value azurerm_public_ip.pip.id so the compute module can receive the Public IP resource id and attach the NIC. Without this, the NIC/VM cannot be associated with the Public IP (checklist: compute must receive public_ip_id).
| output "public_ip_fqdn" { | ||
| description = "The FQDN of the Public IP Address" | ||
| value = azurerm_public_ip.pip.fqdn |
There was a problem hiding this comment.
public_ip_fqdn returning azurerm_public_ip.pip.fqdn is useful for accessing the app in a browser, but it does not replace the need for the Public IP resource id. Ensure you keep the FQDN output for verification but also add the public_ip_id output. Also verify that the resource name azurerm_public_ip.pip exactly matches the resource defined in modules/network/main.tf so this output references a real resource.
| @@ -0,0 +1,8 @@ | |||
| output "storage_account_name" { | |||
| description = "The name of the Storage Account for application artifacts" | |||
| value = azurerm_storage_account.sa.name | |||
There was a problem hiding this comment.
You export azurerm_storage_account.sa.name. Ensure the storage account resource in modules/storage/main.tf is indeed named azurerm_storage_account.sa. If the resource block uses a different local name the output will fail. Also confirm the storage account name meets Azure naming rules and the task’s expectations (the task requires a storage account to exist; backend uses a separate literal yourstorageaccount).
| } | ||
| output "storage_container_name" { | ||
| description = "The name of the Storage Container for application artifacts" | ||
| value = azurerm_storage_container.sc.name |
There was a problem hiding this comment.
You export azurerm_storage_container.sc.name. Verify the resource in modules/storage/main.tf is named azurerm_storage_container.sc and that the container's name is exactly task-artifacts (the checklist mandates the storage container name be task-artifacts). If the container name differs the solution will not meet the requirements.
| output "storage_account_name" { | ||
| description = "The name of the Storage Account for application artifacts" | ||
| value = azurerm_storage_account.sa.name | ||
| } | ||
| output "storage_container_name" { | ||
| description = "The name of the Storage Container for application artifacts" | ||
| value = azurerm_storage_container.sc.name | ||
| } No newline at end of file |
There was a problem hiding this comment.
Critical: The root main.tf passes script_url = module.storage.script_url into the compute module, but this module does not define an output named script_url. Add an output that provides the reachable URL to install-app.sh (for example from an azurerm_storage_blob resource's url attribute or by constructing the blob endpoint). The compute module’s Custom Script extension must be able to download and run install-app.sh from this URL — without that output the VM extension cannot run the application installer (HIGH severity).
| description = "Name of the storage account for application artifacts" | ||
| type = string | ||
| } | ||
| variable "storage_container_name" { |
There was a problem hiding this comment.
Consider enforcing the checklist requirement that the storage container name must be exactly task-artifacts. You have storage_container_name typed as string but no default. To guarantee compliance you can set default = "task-artifacts" here, or ensure the root-level variable storage_container_name_app_artifacts (used when instantiating this module) has that exact value. The checklist mandates that the created container name is task-artifacts.
| description = "Azure region for resources" | ||
| type = string | ||
| } | ||
| variable "storage_account_name" { |
There was a problem hiding this comment.
The storage_account_name variable is fine as a string, but storage account names must follow Azure rules (lowercase, 3-24 chars, letters/numbers). Ensure the value passed from root (var.storage_account_name_app_artifacts) meets those constraints (the task allows any valid name). You may want to add a brief note or validation rule here to prevent invalid values being supplied.
| @@ -0,0 +1,13 @@ | |||
| resource "azurerm_storage_account" "sa" { | |||
| name = var.storage_account_name | |||
There was a problem hiding this comment.
The storage account resource is defined but you must ensure var.storage_account_name produces a valid, unique Azure storage account name (lowercase, 3-24 characters, alphanumeric). The task allows any valid name, but in practice you should either set a compliant default or append a random suffix to guarantee uniqueness.
| } | ||
|
|
||
| resource "azurerm_storage_container" "sc" { | ||
| name = var.storage_container_name |
There was a problem hiding this comment.
The checklist requires the storage container name to be exactly task-artifacts. Here the container name comes from var.storage_container_name. Make sure the variable default (or the value passed from root via storage_container_name_app_artifacts) equals task-artifacts exactly; otherwise this will not meet the requirement.
| resource "azurerm_storage_container" "sc" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.sa.name | ||
| container_access_type = "blob" | ||
| } No newline at end of file |
There was a problem hiding this comment.
The root main.tf passes module.storage.script_url into the compute module so the VM extension can run install-app.sh. This main.tf does not create a blob for install-app.sh nor does it produce an output named script_url. You need to add an azurerm_storage_blob (or another mechanism) to upload install-app.sh, and expose a script_url output (public blob URL or SAS URL) so the compute module can download and execute the script (checklist: CustomScript must run install-app.sh).
| resource "azurerm_storage_container" "sc" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.sa.name | ||
| container_access_type = "blob" | ||
| } No newline at end of file |
There was a problem hiding this comment.
If the blob is private, the compute module will need a SAS or other authenticated URL to download install-app.sh. Plan to either (1) create the blob as publicly accessible and return its URL, or (2) generate a SAS token (for example using azurerm_storage_account_sas) and output a full script_url including the SAS. Without this, the VM extension will not be able to fetch the script.
| @@ -0,0 +1,3 @@ | |||
| location = "uksouth" | |||
There was a problem hiding this comment.
Good: location is set to "uksouth", matching the requirement that resources be created in the uksouth region. Ensure variables.tf also declares location so this value is applied.
| @@ -0,0 +1,3 @@ | |||
| location = "uksouth" | |||
| resource_group_name = "mate-azure-task-12" | |||
There was a problem hiding this comment.
Good: resource_group_name is set to "mate-azure-task-12", which must be the resource group name per the checklist. Ensure the root variables.tf uses the same variable name so this value overrides the default as intended.
| @@ -0,0 +1,3 @@ | |||
| location = "uksouth" | |||
| resource_group_name = "mate-azure-task-12" | |||
| ssh_key_public = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCnTufDrlK40pNPp2Qho8Z7aixGNbV7q1hkfJzIUcQiYsr5eN1Utc/zx1E6IPDyHi+t4E5GKDm88yU8FbeDL3ShAULkB4/UvQzmTho2DjHRBEfAo9Z/pPkv0HJYtlgdol0aCwo30vWr/dRtq6Ir2Iw5hAZcAe3hKg2Qq9VfjBjiOTl434ONar0wajnApA550nuWEKU/vvQEBdP8Lx+vulteTGmvP9JWV4ZhWQVJkArYi0wvRX1UEoEj/P/eXNQl+h7BwxGRa3OyUaU7yCRE0QKba0mesAwSBdncs+tFqoNQD7JiCR7QnSzKoQ7MJQg/945d86UijPisT9IGGDyyB9kZrEUR/ePrCuH+xhFXuOdHectdH3xzui00zhdoiov6XfTYMrzoOF1+aJJFMI42yySGV5lWknfvIfAAr/1j6/1HPAIX/FDnlUPRRuMMLWHUAguW/oqXIRKzpR6h4/YVCe/cToHpPIkd3jZpCNADKXkp4Eoef78lOn1S71gVjzk1LOk=" No newline at end of file | |||
There was a problem hiding this comment.
ssh_key_public contains the public key string and matches the variable name used in root/main.tf. Ensure the compute module consumes this variable to create the SSH key for the VM (the checklist expects an SSH key named linuxboxsshkey). Also note: this tfvars file does not include values for other variables the configuration references (for example: dns_label, storage_account_name_app_artifacts, storage_container_name_app_artifacts, vm_name, vm_size, vnet_address_prefix, subnet_address_prefix). If those variables do not have defaults in variables.tf, you should add them here so terraform apply runs non-interactively.
| variable "ssh_key_public" { | ||
| description = "Public SSH key content for VM access" | ||
| type = string | ||
| sensitive = true # Позначаємо як чутливу інформацію | ||
| } |
There was a problem hiding this comment.
The checklist requires the root variable ssh_key_public to be declared with the default value your-public-key-content. Here variable "ssh_key_public" is missing a default. Add default = "your-public-key-content" (or ensure terraform.tfvars sets that exact value) so the root variables match the task requirements. Note: you may keep sensitive = true if you prefer, but the default must still be present to satisfy the checklist.
| @@ -0,0 +1,7 @@ | |||
| output "public_ip_address" { | |||
| value = module.network.public_ip_address | |||
There was a problem hiding this comment.
This output references module.network.public_ip_address. Make sure modules/network/outputs.tf defines an output named public_ip_address (for example returning azurerm_public_ip.pip.ip_address). If that output does not exist you will get an error at plan/app time. Also ensure the network module still provides public_ip_id (used in root main.tf to wire into compute).
| } | ||
|
|
||
| output "public_ip_fqdn" { | ||
| value = module.network.public_ip_fqdn |
There was a problem hiding this comment.
This output references module.network.public_ip_fqdn. Ensure modules/network/outputs.tf defines public_ip_fqdn (for example returning azurerm_public_ip.pip.fqdn or the correct attribute from the public IP resource). Verify the FQDN is available (non-empty) after provisioning and that the DNS label in the network module is constructed per the checklist (matetask + random suffix).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you progress while learning. You’ve structured the modules well and covered the key pieces (networking, VM with CustomScript, and storage uploading the script). To strengthen reliability and meet requirements, please fix these critical items: 1) in modules/compute/outputs.tf, make the public IP data lookup conditional and avoid brittle split(...)[8]; prefer passing the public IP name/id from the network module or dynamically extracting the last segment, 2) in main.tf, pass list variables directly (remove the extra brackets around vnet_address_prefix and subnet_address_prefix), and 3) align storage variable names between root and the storage module (storage_account_name / storage_container_name) and ensure install-app.sh path matches your intent. Also replace the backend placeholder storage account with a real one and run terraform fmt -recursive to fix CI formatting. Great job on NSG rules, outputs, and the VM setup—once you apply these fixes, your deployment should plan and apply cleanly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| output "vm_public_ip" { | ||
| value = azurerm_network_interface.main.ip_configuration[0].public_ip_address_id != null ? data.azurerm_public_ip.vm_pip.ip_address : "" |
There was a problem hiding this comment.
The ternary expression here depends on data.azurerm_public_ip.vm_pip succeeding, but the data block below will still be evaluated even when public_ip_address_id is null/empty. That can cause plan/apply to fail. Consider one of these safer approaches:
- Pass the public IP name or id from the network module into the compute module and use it directly (preferred).
- Make the
datalookup conditional (usecount/for_each) so it is only evaluated when a public IP id/name exists.
This change will make thevm_public_ipoutput reliable and avoid errors when the NIC/public IP is not set at evaluation time.
| data "azurerm_public_ip" "vm_pip" { | ||
| name = split("/", azurerm_network_interface.main.ip_configuration[0].public_ip_address_id)[8] | ||
| resource_group_name = var.resource_group_name | ||
| depends_on = [azurerm_linux_virtual_machine.main] |
There was a problem hiding this comment.
This data.azurerm_public_ip block derives the public IP name with split(...)[8], which is fragile — it assumes the resource ID always has at least 9 segments and that the name is at index 8. This can break across different ID formats or if additional path segments appear. Recommended alternatives:
- Pass the public IP name (or resource id) from the network module into compute (best), so you can look it up with
databy name safely. - If you must parse the ID, extract the last segment dynamically, e.g.
element(split("/", azurerm_network_interface.main.ip_configuration[0].public_ip_address_id), length(split("/", azurerm_network_interface.main.ip_configuration[0].public_ip_address_id)) - 1)to get the final element instead of hardcoding index 8.
Also ensure thedatablock is only evaluated when thepublic_ip_address_idis present (see previous comment).
| name = "internal" | ||
| subnet_id = var.subnet_id | ||
| private_ip_address_allocation = "Dynamic" | ||
| public_ip_address_id = var.public_ip_id |
There was a problem hiding this comment.
The NIC's ip_configuration sets public_ip_address_id = var.public_ip_id. The network module must export public_ip_id (the Public IP resource id) so this receives a valid resource id; the required output is output "public_ip_id" { value = azurerm_public_ip.pip.id }. Make sure modules/network/outputs.tf provides that output exactly so the NIC can be associated to the public IP as expected by the checklist .
| 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.
You associate the NSG to the NIC using var.nsg_id. Confirm the network module creates a Network Security Group named defaultnsg and exports nsg_id (azurerm_network_security_group.nsg.id). Also ensure that NSG contains inbound rules to allow SSH (port 22) and the application port (e.g., HTTP 80) so the VM is reachable from the Internet (the compute module relies on that association) .
|
|
||
| # Creates the Linux Virtual Machine. | ||
| resource "azurerm_linux_virtual_machine" "main" { | ||
| name = var.vm_name |
There was a problem hiding this comment.
The VM name is set to var.vm_name. The task requires the VM be named matebox. That is acceptable if var.vm_name is set to matebox in root variables/terraform.tfvars (confirm that default/value exists). If you want to guarantee the exact required name regardless of variable overrides, consider setting name = "matebox" or ensure the root vm_name variable default/value is matebox .
| settings = <<SETTINGS | ||
| { | ||
| "fileUris": ["${var.script_url}"], |
There was a problem hiding this comment.
The CustomScript extension uses fileUris with ${var.script_url} and commandToExecute to run install-app.sh. Make sure the storage module uploads install-app.sh and outputs script_url (a URL accessible to the VM) — e.g. azurerm_storage_blob.app_script.url. Also verify the blob/container access settings make the blob reachable (modules/storage sets container_access_type = "blob" which allows blob read access). If the blob is private you must supply a SAS URL instead. See modules/storage main/outputs for the expected script_url output .
| resource_group_name = azurerm_resource_group.rg.name | ||
| location = azurerm_resource_group.rg.location | ||
| virtual_network_name = var.virtual_network_name | ||
| vnet_address_prefix = [var.vnet_address_prefix] |
There was a problem hiding this comment.
You are passing the VNet address prefix wrapped in an extra list: vnet_address_prefix = [var.vnet_address_prefix]. In your root variables vnet_address_prefix is already declared as a list(string) and the network module expects list(string) as well, so wrapping it again will produce a nested list and Terraform type errors. Pass the variable directly: vnet_address_prefix = var.vnet_address_prefix. See modules/network/variables.tf which declares the input type as list(string) and the root variables where the default is already a list .
| 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] |
There was a problem hiding this comment.
Same problem for the subnet address prefix: you’re sending [var.subnet_address_prefix] but subnet_address_prefix in the root variables is already a list(string) and the network module expects a list(string) — remove the extra [] and pass var.subnet_address_prefix directly to avoid a nested-list type mismatch .
| 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 called with storage_account_name = var.storage_account_name and storage_container_name = var.storage_container_name, but your root variables define storage_account_name_app_artifacts and storage_container_name_app_artifacts (not storage_account_name/storage_container_name). This name mismatch will cause Terraform to fail with an undefined variable error. Either rename the root variables to storage_account_name / storage_container_name or change these module arguments to use the _app_artifacts variable names. See the root variables file for the actual names defined .
| subnet_id = module.network.subnet_id | ||
| public_ip_id = module.network.public_ip_id |
There was a problem hiding this comment.
Verify the network module implements the outputs that the compute module expects (subnet_id, public_ip_id, nsg_id). The compute module here uses module.network.subnet_id, module.network.public_ip_id, and module.network.nsg_id so those outputs must exist and be named exactly in modules/network/outputs.tf (I see outputs expecting azurerm_public_ip.pip.id etc. in the network outputs file; ensure the network resources are named accordingly in modules/network/main.tf) .
| public_ip_id = module.network.public_ip_id | ||
| nsg_id = module.network.nsg_id | ||
| script_url = module.storage.script_url | ||
| } |
There was a problem hiding this comment.
After you fix the variable names and list types, run terraform init (to ensure modules metadata is correct) and terraform fmt -recursive (to satisfy formatting checks). If you get a modules.json duplicate/empty entry warning re-run terraform init and inspect .terraform/modules/modules.json as suggested in the previous review.
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "yourstorageaccount" | ||
| container_name = "tfstate" | ||
| key = "terraform.tfstate" |
There was a problem hiding this comment.
The backend block is syntactically correct, but storage_account_name = "yourstorageaccount" is a placeholder. Replace this with the actual storage account name that exists in the mate-azure-task-12 resource group, or ensure the storage account and the tfstate container are created prior to running terraform init. The backend requires the storage account and container to exist and be accessible at init time.
| name = var.virtual_network_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| address_space = var.vnet_address_prefix |
There was a problem hiding this comment.
address_space = var.vnet_address_prefix — This is correct because the network module declares vnet_address_prefix as list(string) in modules/network/variables.tf. Make sure the root module passes the variable as a list (without wrapping it in an extra []). The root currently calls vnet_address_prefix = [var.vnet_address_prefix], which would create a nested list and cause a type error; pass vnet_address_prefix = var.vnet_address_prefix from root instead .
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefix |
There was a problem hiding this comment.
address_prefixes = var.subnet_address_prefix — same guidance as above: the subnet variable is defined as list(string) in the module's variables, so ensure the root passes it directly (not wrapped in another list). If the root wraps it you will get a nested-list type mismatch at plan time .
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| domain_name_label = "${var.dns_label}${random_integer.r.result}" |
There was a problem hiding this comment.
domain_name_label = "${var.dns_label}${random_integer.r.result}" — using a random suffix here satisfies the DNS-label-with-randomness requirement. However, this module uses the random provider (resource random_integer). Ensure the root required_providers includes random (or document that the root must declare it). Without the provider available you may get initialization warnings or unexpected behavior. Adding random to the root required_providers block is the usual approach.
| resource "random_integer" "r" { | ||
| min = 10000 | ||
| max = 99999 | ||
| } No newline at end of file |
There was a problem hiding this comment.
resource "random_integer" "r" { min = 10000 max = 99999 } — this produces a 5-digit suffix which is fine. Declaring the random resource after the public IP reference is acceptable (Terraform resolves cross-resource references), so this placement is OK if you prefer it here.
| resource "azurerm_network_security_group" "nsg" { | ||
| name = var.network_security_group_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name |
There was a problem hiding this comment.
Network Security Group: you defined inbound rules for SSH and HTTP which make the VM reachable — good. Note that the module does not attach the NSG to the subnet here; that’s acceptable because the compute module associates the NSG to the NIC via azurerm_network_interface_security_group_association (the compute module expects module.network.nsg_id). Ensure modules/network/outputs.tf exports nsg_id (it does) and that the compute module consumes it exactly as named (verify wiring in root main.tf) .
| variable "vnet_address_prefix" { | ||
| description = "Префікс адреси для віртуальної мережі." | ||
| type = list(string) |
There was a problem hiding this comment.
vnet_address_prefix is declared as list(string), which is correct for this module. When calling this module from root, pass the list directly (e.g. vnet_address_prefix = var.vnet_address_prefix) — do not wrap it again like [var.vnet_address_prefix] (that creates a nested list and produces a type error).
| variable "subnet_address_prefix" { | ||
| description = "Префікс адреси для підмережі." | ||
| type = list(string) |
There was a problem hiding this comment.
Same as above for subnet_address_prefix: it is declared list(string) here and should be invoked with the list directly from root (e.g. subnet_address_prefix = var.subnet_address_prefix). Avoid passing [var.subnet_address_prefix] which creates a nested list and will fail type checking.
| # The subnet ID. | ||
| output "subnet_id" { | ||
| description = "The ID of the subnet." | ||
| value = azurerm_subnet.subnet.id |
There was a problem hiding this comment.
This output references azurerm_subnet.subnet. Confirm that modules/network/main.tf defines the subnet resource with the local name subnet (for example resource "azurerm_subnet" "subnet" { ... }). If the subnet resource is named differently the output will fail — either rename the resource or update this output to match the actual resource name.
| output "public_ip_id" { | ||
| description = "The ID of the Public IP Address." | ||
| value = azurerm_public_ip.pip.id |
There was a problem hiding this comment.
Good — public_ip_id is present (this was a HIGH-priority fix). Ensure modules/network/main.tf defines the public IP as resource "azurerm_public_ip" "pip" { ... } and configures allocation_method = "Dynamic" and a DNS label that appends a random suffix (e.g. using random_integer or random_id) so the pip.id value exists and can be consumed by the NIC/compute module.
| output "nsg_id" { | ||
| description = "The ID of the Network Security Group." | ||
| value = azurerm_network_security_group.nsg.id |
There was a problem hiding this comment.
This output uses azurerm_network_security_group.nsg.id. Ensure the network module defines the NSG with the local name nsg (resource "azurerm_network_security_group" "nsg" { ... }) and that the NSG contains inbound rules for SSH (port 22) and the application port (e.g. port 80) so the VM is reachable over the internet once associated with the NIC.
| output "public_ip_address" { | ||
| description = "The Public IP Address of the Linux VM." | ||
| value = azurerm_public_ip.pip.ip_address | ||
| } | ||
|
|
||
| # The FQDN of the Public IP Address. | ||
| output "public_ip_fqdn" { | ||
| description = "The FQDN of the Public IP Address." | ||
| value = azurerm_public_ip.pip.fqdn |
There was a problem hiding this comment.
public_ip_address and public_ip_fqdn rely on attributes from azurerm_public_ip.pip. Remember that fqdn is only populated when a dns_label is set on the public IP. Verify the public IP resource sets the DNS label (and uses randomness appended to matetask) so public_ip_fqdn is valid and the FQDN can be returned here.
| output "public_ip_fqdn" { | ||
| description = "The FQDN of the Public IP Address." | ||
| value = azurerm_public_ip.pip.fqdn | ||
| } |
There was a problem hiding this comment.
If modules/network/main.tf is missing (or defines different resource local names), these outputs will break. Double-check that the network module exists and defines azurerm_subnet.subnet, azurerm_public_ip.pip, and azurerm_network_security_group.nsg exactly as referenced here. If names differ, update either the network resources or these outputs to match.
| @@ -0,0 +1,24 @@ | |||
| # Creates the storage account for application artifacts. | |||
| resource "azurerm_storage_account" "sa" { | |||
| name = var.storage_account_name | |||
There was a problem hiding this comment.
The module expects var.storage_account_name here. The repository's root variables file defines storage_account_name_app_artifacts instead — either change the root invocation to pass storage_account_name = var.storage_account_name_app_artifacts, or rename the root variable to storage_account_name. Without this fix Terraform will report an undefined variable when you call the module from the root.
|
|
||
| # Creates the storage container for application artifacts. | ||
| resource "azurerm_storage_container" "sc" { | ||
| name = var.storage_container_name |
There was a problem hiding this comment.
The container name is taken from var.storage_container_name. The root variables define storage_container_name_app_artifacts by default. Make sure the root module passes the correct variable to match this module (e.g. storage_container_name = var.storage_container_name_app_artifacts) or rename the module input to match the root. This mismatch will cause an undefined-variable error at plan time.
| resource "azurerm_storage_container" "sc" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.sa.name | ||
| container_access_type = "blob" |
There was a problem hiding this comment.
container_access_type = "blob" makes blobs publicly accessible (readable). This is necessary so the VM extension can download the script using the blob URL returned by azurerm_storage_blob.app_script.url. Confirm this level of public access is acceptable for your environment (it matches the task requirement to make the script downloadable by the VM) .
| storage_account_name = azurerm_storage_account.sa.name | ||
| storage_container_name = azurerm_storage_container.sc.name | ||
| type = "Block" | ||
| source = "${path.root}/install-app.sh" |
There was a problem hiding this comment.
source = "${path.root}/install-app.sh" — ensure install-app.sh exists at the repository root. If the script actually lives inside this module directory, prefer source = "${path.module}/install-app.sh". If the file is missing, Terraform will fail to upload the blob and the compute module will not be able to download the script (the CustomScript extension depends on module.storage.script_url).
| @@ -0,0 +1,24 @@ | |||
| # Creates the storage account for application artifacts. | |||
| resource "azurerm_storage_account" "sa" { | |||
| name = var.storage_account_name | |||
There was a problem hiding this comment.
Note: storage account names must be globally unique and lowercase. The module uses var.storage_account_name as provided; ensure the value you pass meets Azure rules (e.g. matetaskappstorage may be okay but might collide). Consider documenting this requirement in the module's README or variable description.
| @@ -0,0 +1,3 @@ | |||
| location = "uksouth" | |||
| resource_group_name = "mate-azure-task-12" | |||
| ssh_key_public = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCnTufDrlK40pNPp2Qho8Z7aixGNbV7q1hkfJzIUcQiYsr5eN1Utc/zx1E6IPDyHi+t4E5GKDm88yU8FbeDL3ShAULkB4/UvQzmTho2DjHRBEfAo9Z/pPkv0HJYtlgdol0aCwo30vWr/dRtq6Ir2Iw5hAZcAe3hKg2Qq9VfjBjiOTl434ONar0wajnApA550nuWEKU/vvQEBdP8Lx+vulteTGmvP9JWV4ZhWQVJkArYi0wvRX1UEoEj/P/eXNQl+h7BwxGRa3OyUaU7yCRE0QKba0mesAwSBdncs+tFqoNQD7JiCR7QnSzKoQ7MJQg/945d86UijPisT9IGGDyyB9kZrEUR/ePrCuH+xhFXuOdHectdH3xzui00zhdoiov6XfTYMrzoOF1+aJJFMI42yySGV5lWknfvIfAAr/1j6/1HPAIX/FDnlUPRRuMMLWHUAguW/oqXIRKzpR6h4/YVCe/cToHpPIkd3jZpCNADKXkp4Eoef78lOn1S71gVjzk1LOk=" No newline at end of file | |||
There was a problem hiding this comment.
The compute/root configuration requires an SSH public key value and you have provided it here. Note the root variables.tf declares ssh_key_public as sensitive = true so avoid committing credentials directly to the repository in production — consider using environment variables, a CI secret, or a separate (gitignored) tfvars file for secrets. See the root variables declaration for ssh_key_public which marks it sensitive .
| location = "uksouth" | ||
| resource_group_name = "mate-azure-task-12" |
There was a problem hiding this comment.
The root main.tf invokes the storage module using arguments storage_account_name and storage_container_name (so the module call expects those variable names), but your root variables file defines storage_account_name_app_artifacts and storage_container_name_app_artifacts. To avoid undefined-variable errors you must either: (A) add storage_account_name and storage_container_name entries to this terraform.tfvars, or (B) change the root module invocation to pass the _app_artifacts variables. See the root module invocation in main.tf and the storage module variables for the mismatch .
| variable "vnet_address_prefix" { | ||
| description = "Address prefix for the Virtual Network" | ||
| type = list(string) | ||
| default = ["10.0.0.0/16"] |
There was a problem hiding this comment.
vnet_address_prefix is declared as list(string) — this is correct. However the root module call is currently passing it wrapped in another list (vnet_address_prefix = [var.vnet_address_prefix] in main.tf) which creates a nested list and will cause a type error at plan time. From the variable declaration you should pass var.vnet_address_prefix directly from root (no extra []). See the module invocation in main.tf for how it's currently passed.
| variable "subnet_address_prefix" { | ||
| description = "Address prefix for the Subnet" | ||
| type = list(string) | ||
| default = ["10.0.0.0/24"] |
There was a problem hiding this comment.
subnet_address_prefix is declared as list(string) (good). Same issue as above: ensure the root passes it as var.subnet_address_prefix (not [var.subnet_address_prefix]) — otherwise Terraform will see a nested list and error. Confirm and fix the root main.tf accordingly.
| variable "ssh_key_public" { | ||
| description = "Public SSH key content for VM access" | ||
| type = string | ||
| sensitive = true # Позначаємо як чутливу інформацію | ||
| } |
There was a problem hiding this comment.
The variable ssh_key_public is declared (sensitive) but has no default value here. The task/checklist indicates an example/default value (your-public-key-content) should be present in variables.tf (the repository does set a value in terraform.tfvars, but the checklist asked for the variables.tf to contain the listed defaults). Please either add a harmless example default (e.g. default = "your-public-key-content") or ensure your README/terraform.tfvars clearly documents that the value must be provided. terraform.tfvars currently provides a value — see that file if you prefer to keep the value out of variables.tf.
| variable "storage_account_name_app_artifacts" { | ||
| description = "Name of the storage account for application artifacts" | ||
| type = string | ||
| default = "matetaskappstorage" # Унікальне ім'я |
There was a problem hiding this comment.
Mismatch: this file defines storage_account_name_app_artifacts, but the storage module (and the root invocation) expects storage_account_name. The storage module's variables are storage_account_name / storage_container_name and its main.tf uses var.storage_account_name — either rename these root variables to storage_account_name/storage_container_name or change the root module call to pass storage_account_name = var.storage_account_name_app_artifacts. As-is Terraform will report an undefined variable when calling the storage module.
| variable "storage_container_name_app_artifacts" { | ||
| description = "Name of the storage container for application artifacts" | ||
| type = string | ||
| default = "task-artifacts" |
There was a problem hiding this comment.
Same as previous: storage_container_name_app_artifacts here does not match the storage_container_name variable expected by the storage module. Change the names to be consistent between root variables, root module invocation, and the storage module inputs/variables. This must be fixed for the storage module to receive correct values and to produce module.storage.script_url for the compute module.
Experimental 3
No description provided.