Should be ready!#71
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 several core requirements from the task are not yet met and they will block a correct deployment and automated checks.
Critical blockers that must be fixed:
- The Terraform backend is configured with
storage_account_name = "mateback12storage"instead of the required"yourstorageaccount"(e.g. inbackend.tfand related backend blocks), and you also hard‑code asubscription_id; this violates the precise backend spec and can break validation. - The root
variables.tfdoes not define the full set of required variables (VNet/subnet/NSG/public IP/VM/dns_label) and uses the wrong default location (denmarkeastinstead ofuksouth); the compute module also defaultsvm_sizetoStandard_DS1_v2rather than the requiredStandard_B1s. - The public IP in
modules/network/main.tfis configured asallocation_method = "Static"with a default namepublicip, but the task requires a dynamically allocated public IP namedlinuxboxpipwith DNS labelmatetask+ random number; this must be corrected (either via defaults or explicit root overrides). - SSH key handling treats
ssh_key_publicas a file path and usesfile(var.ssh_key_public), while the requirement is thatssh_key_publicrepresents the public key content for thelinuxboxsshkeykey; you should switch the variable to hold content and usepublic_key = var.ssh_key_public. - The ToDo app script
install-app.shis never uploaded to thetask-artifactscontainer, so the CustomScript extension’sfileUrisURL will 404 and the app will not actually deploy; add a blob upload resource or document a manual upload step that matches the URL. - Your public IP output currently relies on
azurerm_linux_virtual_machine.vm.public_ip_address, which is not a valid attribute in azurerm 4.x; instead, output the IP from theazurerm_public_ipresource (e.g.linuxboxpip.ip_address) and surface that at root.
Minor but still worth fixing: run terraform fmt -recursive to satisfy the formatting check, consider aligning module defaults like subnet_name to "default", and remove the hard‑coded backend subscription_id. Once you align these names, defaults, and script hosting with the specification, your overall structure (modules, resources, extension) will be in very good shape.
✨ 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
| # Local .terraform directories | ||
| **/.terraform/* | ||
|
|
||
| # .tfstate files |
There was a problem hiding this comment.
Per the task, the backend must use storage_account_name = "yourstorageaccount", but this currently uses "mateback12storage". Update this to the exact required value and make sure the corresponding storage account and container exist.
| # .tfstate files | ||
| *.tfstate | ||
| *.tfstate.* | ||
|
|
There was a problem hiding this comment.
The description does not mention or require setting subscription_id in the backend block; hard‑coding an ID here is unnecessary and can cause problems when others run this configuration. Consider removing it and relying on standard Azure auth/CLI context instead.
| # Local .terraform directories | ||
| **/.terraform/* | ||
|
|
||
| # .tfstate files |
There was a problem hiding this comment.
The assignment expects the backend storage account name to be yourstorageaccount, not mateback12storage. This mismatch means your remote state configuration does not comply with the requirements.
|
|
||
| # Override files | ||
| override.tf | ||
| override.tf.json |
There was a problem hiding this comment.
Here you hard‑code vnet_name = "vnet", but you are not passing the required address prefix variables (10.0.0.0/16 and 10.0.0.0/24). Ensure the module inputs (or defaults) produce exactly the VNet/subnet configuration described in the task.
| # Override files | ||
| override.tf | ||
| override.tf.json | ||
| *_override.tf |
There was a problem hiding this comment.
You are not passing public_ip_address_name or dns_label into the network module. The requirements specify a public IP named linuxboxpip with DNS label matetask + random number, so make sure the module variables and overrides enforce these names.
| .terraformrc | ||
| terraform.rc | ||
|
|
||
| # Terraform lock file (optional - some teams commit this) |
There was a problem hiding this comment.
The storage account created here is mateazuretask12storage, but the backend configuration uses a different account name. The task requires the backend to use a specific storage account yourstorageaccount, so align these or document that the backend account is managed separately as per the description.
| provider "registry.terraform.io/hashicorp/azurerm" { | ||
| version = "4.76.0" | ||
| hashes = [ | ||
| "h1:s6/CZvCarsCPchXCgyY6s9kMngomo7nYQyzm9HaqyjY=", |
There was a problem hiding this comment.
The backend configuration must match the task description exactly. Here, storage_account_name is "mateback12storage", but the description requires "yourstorageaccount", and it does not mention subscription_id. Using a different name will break automated validation, and committing a real subscription ID is not expected for this exercise.
| "zh:07bb3a087abca8bd14cc28e3d3d4abb71e0ed711ecec65e7c0a2f1a97b948cf4", | ||
| "zh:0fa6640b5d6c0b0fe0b7fa25cebc0091b7dda2efd83ec0347c5a50dfce0957bc", | ||
| "zh:3c8618c8a5ab07fadfcb53ecf614f7b99e25bb6c407cf0af703a6b0647d4461d", | ||
| "zh:5f23fdda6ff290bc99b25b68f612bf67fbb503b04b1e38664e66d3204a51e99e", |
There was a problem hiding this comment.
The course description requires the default location variable to be uksouth. Using denmarkeast here violates that requirement and may cause automated checks to fail. Adjust the default to uksouth to align with the spec.
| provider "azurerm" { | ||
| features { | ||
| resource_group { | ||
| prevent_deletion_if_contains_resources = false |
There was a problem hiding this comment.
The task requires the backend configuration to use storage_account_name = "yourstorageaccount", but this is set to "mateback12storage". Update this to match the exact required value so your backend configuration complies with the assignment.
| module "network" { | ||
| source = "./modules/network" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = "vnet" | ||
| subnet_name = "default" |
There was a problem hiding this comment.
According to the description, VNet name, address prefix, subnet name/prefix, NSG name, and public IP name/behavior should be driven by variables defined in variables.tf (e.g., virtual_network_name, vnet_address_prefix, etc.). Here you only pass vnet_name and subnet_name, and they’re hardcoded; consider wiring all required network-related variables from root to the module to match the spec.
| module "storage" { | ||
| source = "./modules/storage" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| storage_account_name = "mateazuretask12storage" | ||
| storage_container_name = "task-artifacts" |
There was a problem hiding this comment.
storage_account_name is hardcoded here, but the task expects a variables.tf that defines required variables for configuration (and a storage module that can accept them). Ensure this value and others align with the required variables and naming conventions from the description (e.g., use variables instead of hardcoding where the description mandates variable definitions).
| module "compute" { | ||
| source = "./modules/compute" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vm_name = "matebox" | ||
| vm_size = "Standard_B1s" | ||
| admin_username = "azureuser" | ||
| ssh_key_public = var.ssh_key_public |
There was a problem hiding this comment.
The VM name and size are correct per requirements (matebox, Standard_B1s), but the assignment also specifies an SSH key named linuxboxsshkey and a ssh_key_public variable representing the key content. Make sure your SSH key handling (path vs content and key name) matches those requirements throughout the configuration, not only via this variable reference.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| size = var.vm_size | ||
| admin_username = var.admin_username |
There was a problem hiding this comment.
The VM size default here is "Standard_DS1_v2", but the task explicitly requires vm_size: Standard_B1s in variables.tf. Update this default to "Standard_B1s" so the module matches the assignment’s required configuration.
| admin_ssh_key { | ||
| username = var.admin_username | ||
| public_key = file(var.ssh_key_public) |
There was a problem hiding this comment.
The description says ssh_key_public should represent the public key content (your-public-key-content), not a file path. Using file(var.ssh_key_public) assumes a path. Consider changing the variable to hold the content and use public_key = var.ssh_key_public to align with the spec and avoid hidden file dependencies.
| resource "azurerm_network_interface" "nic" { | ||
| name = "${var.vm_name}-nic" |
There was a problem hiding this comment.
Depending on your azurerm provider version, azurerm_linux_virtual_machine.vm may not expose public_ip_address directly. If Terraform errors on this attribute, you’ll need to export the IP from the azurerm_public_ip resource instead and wire it here or from the network module.
| output "public_ip_vm" { | ||
| value = azurerm_linux_virtual_machine.vm.public_ip_address |
There was a problem hiding this comment.
azurerm_linux_virtual_machine does not expose a public_ip_address field; the public IP is a separate resource associated via the NIC. This output will be invalid. Instead, expose the public IP from the azurerm_public_ip resource in the network module and output that from root.
| variable "vm_size" { | ||
| description = "The size of the virtual machine." | ||
| type = string | ||
| default = "Standard_DS1_v2" |
There was a problem hiding this comment.
This vm_size default conflicts with the task description, which requires VMs to be Standard_B1s. Update the default to "Standard_B1s" so the module matches the required configuration even when callers don’t override it.
| description = "The admin username for the virtual machine." | ||
| type = string | ||
| } | ||
|
|
There was a problem hiding this comment.
The description says ssh_key_public should represent the public key content (your-public-key-content), but here it is treated as a file path (file(var.ssh_key_public)). Consider changing the variable to hold the actual key content and pass public_key = var.ssh_key_public to match the spec.
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefixes | ||
| } | ||
|
|
There was a problem hiding this comment.
The default vm_size in the module is Standard_DS1_v2, but the task explicitly requires Standard_B1s. Even though the root module overrides this, the module’s default should also be aligned with the required size.
|
|
||
| resource "azurerm_network_security_group" "defaultnsg" { | ||
| name = var.nsg_name |
There was a problem hiding this comment.
Here you treat ssh_key_public as a file path and call file(...), but the description says ssh_key_public should represent the public key content. Consider making the variable hold the key content directly and removing file(), and ensure it corresponds to the linuxboxsshkey key as required.
| resource "random_integer" "dns_suffix" { | ||
| min = 10000 |
There was a problem hiding this comment.
The azurerm_linux_virtual_machine resource does not expose a public_ip_address attribute directly. This output will fail because Terraform can’t find that attribute. You should obtain the IP address from the associated azurerm_public_ip resource (likely output from the network module) instead of from the VM.
| name = var.public_ip_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
The public IP allocation must be dynamic per the assignment, but this is set to "Static". Change this to "Dynamic" to match the requirement.
| resource "azurerm_public_ip" "linuxboxpip" { | ||
| name = var.public_ip_name |
There was a problem hiding this comment.
The description requires the public IP resource to be named linuxboxpip. Here you use var.public_ip_name, whose default in variables is "publicip". Ensure this variable (or default) is set to "linuxboxpip" or override it from the root module so the name matches the spec.
| output "subnet_id" { | ||
| description = "The ID of the created subnet." |
There was a problem hiding this comment.
Instead of hard-coding task-artifacts in this URL, consider using the container variable (e.g. var.storage_container_name) so the output always matches the actual container used and stays in sync with storage_container_name.
| variable "public_ip_name" { | ||
| description = "The name of the public IP address to create." | ||
| type = string | ||
| default = "publicip" |
There was a problem hiding this comment.
The default for public_ip_name is "publicip", but the task requires the public IP resource to be named linuxboxpip. Either change this default to "linuxboxpip" or ensure it is always overridden from the root module so the effective name matches the spec.
| variable "resource_group_name" { | ||
| type = string |
There was a problem hiding this comment.
This URL hard-codes the container name task-artifacts instead of using var.storage_container_name or the actual container resource name. If the variable changes, the URL will break. Consider building the URL using the container name variable/resource so it stays consistent.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name |
There was a problem hiding this comment.
This URL is hard-coded with the container name task-artifacts, ignoring the storage_container_name variable. More importantly, there is no resource or step here that uploads install-app.sh to that container, so the CustomScript extension will 404 when trying to download the script. You should either upload the script via Terraform or clearly document that it must be uploaded manually for the deployment to work.
| output "script_url" { | ||
| value = "https://${azurerm_storage_account.storage.name}.blob.core.windows.net/task-artifacts/install-app.sh" |
There was a problem hiding this comment.
This output assumes that install-app.sh exists in the task-artifacts container, but nothing in the storage module actually uploads that script. Without uploading the file (via Terraform or documented manual steps), the VM’s CustomScript extension will fail to download and execute it.
| type = string | ||
| } | ||
|
|
||
| variable "location" { | ||
| description = "The Azure region in which to create the storage account." |
There was a problem hiding this comment.
The description requires location to default to uksouth, but here it defaults to denmarkeast. Update this default to "uksouth" so it matches the spec and any automated checks.
|
|
||
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the storage account." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The assignment describes ssh_key_public as the public key content (your-public-key-content), but this variable is treated as a file path and defaults to ~/.ssh/linuxboxsshkey.pub. Consider changing it to hold the key content instead of a path to align with the requirements.
| variable "storage_account_name" { | ||
| description = "The name of the storage account." |
There was a problem hiding this comment.
This output relies on module.compute.public_ip_vm, which in turn references vm.public_ip_address. That attribute is not guaranteed to exist; you should instead expose the IP from the azurerm_public_ip resource in the network module and output that here so the public IP used for verification is valid.
| output "public_ip" { | ||
| value = module.compute.public_ip_vm |
There was a problem hiding this comment.
This output depends on module.compute.public_ip_vm, which in turn references azurerm_linux_virtual_machine.vm.public_ip_address. That attribute is not generally exposed by the VM resource in azurerm 4.x; you should instead output the public IP from the azurerm_public_ip resource in the network module and use that here.
| variable "location" { | ||
| description = "The Azure region in which to create the resources." | ||
| type = string | ||
|
|
||
| default = "denmarkeast" |
There was a problem hiding this comment.
The default location is "denmarkeast", but the description explicitly requires location: uksouth in variables.tf. Update this default to "uksouth" to comply with the spec.
| variable "ssh_key_public" { | ||
| description = "The path to the SSH public key to use for authentication." | ||
| type = string | ||
|
|
||
| default = "~/.ssh/linuxboxsshkey.pub" |
There was a problem hiding this comment.
Here ssh_key_public is described and used as a file path, but the task states ssh_key_public: your-public-key-content. Consider redefining this variable to hold the key content and adjusting the compute module to consume it directly, rather than reading a file.
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the resources." |
There was a problem hiding this comment.
This output relies on module.compute.public_ip_vm, which in turn uses vm.public_ip_address. The azurerm_linux_virtual_machine resource generally doesn’t expose public_ip_address in provider 4.x, so this may fail. It would be safer to output the IP from the azurerm_public_ip resource in the network module instead.
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 will cause Terraform to fail or violate explicit task requirements. First, the public IP in modules/network/main.tf is configured with allocation_method = "Static", but the task requires a dynamic public IP [CHECKLIST ITEM #12, #38]; change this to "Dynamic". Second, the public IP address outputs are wired incorrectly: root output.tf references module.network.public_ip_address which doesn’t exist, and modules/compute/outputs.tf references azurerm_public_ip.linuxboxpip even though that resource is not in the compute module; you need to add an output in modules/network/outputs.tf like output "public_ip_address" { value = azurerm_public_ip.linuxboxpip.ip_address } and remove/fix the compute module output [CHECKLIST ITEM #41]. Third, the compute module’s vm_size default in modules/compute/variables.tf is still "Standard_DS1_v2" instead of the required "Standard_B1s", which conflicts with the specification for the default size [CHECKLIST ITEM #10, #37]. Fourth, there is no Terraform resource or clearly documented step that uploads install-app.sh into the task-artifacts container, so the CustomScript extension’s fileUris will 404 and the ToDo app will not deploy [CHECKLIST ITEM #3, #40]; either add a blob upload resource or document the exact manual upload process that matches script_url. As a smaller but required alignment fix, update the compute module’s ssh_key_public description to describe public key content, not a file path, so it matches how you actually use it and the task’s expectations [CHECKLIST ITEM #39].
Once you make these changes, your configuration should both validate successfully and fully meet the remaining functional requirements; the rest of the structure (backend, root variables, modules wiring, NSG/VNet/subnet, VM image, NIC naming, and CustomScript usage) is in good shape and shows solid progress.
✨ 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
| # Local .terraform directories | ||
| **/.terraform/* | ||
|
|
||
| # .tfstate files | ||
| *.tfstate | ||
| *.tfstate.* | ||
|
|
||
| # Crash log files |
There was a problem hiding this comment.
This backend block now matches the required remote state configuration (resource group, storage account, container, key) and no longer hard‑codes a subscription id, which fixes the earlier backend spec issue. No change needed here.
| *.tfvars.json | ||
|
|
||
| # Override files | ||
| override.tf | ||
| override.tf.json | ||
| *_override.tf | ||
| *_override.tf.json | ||
|
|
||
| # CLI configuration files |
There was a problem hiding this comment.
The network module call passes vnet_name and subnet_name, but the module expects vnet_address_prefixes and subnet_address_prefixes as lists; currently nothing passes those, so the module will fall back to its defaults (10.0.0.0/16 and 10.0.0.0/24). If you intend to control these from root vnet_address_prefix / subnet_address_prefix, you should pass them into the module (e.g. wrap the string in a list) to align with checklist item #32.
|
|
||
| # Terraform lock file (optional - some teams commit this) | ||
| # .terraform.lock.hcl | ||
|
|
||
| # SSH keys | ||
| *.pem |
There was a problem hiding this comment.
Using a hard‑coded storage account name here is acceptable for the application artifacts, but note that the backend state uses a different account (yourstorageaccount). Ensure both accounts/containers exist as required: one for Terraform state (tfstate) and one for task-artifacts as per checklist items #5, #27, #30, #31.
| # This file is maintained automatically by "terraform init". | ||
| # Manual edits may be lost in future updates. | ||
|
|
||
| provider "registry.terraform.io/hashicorp/azurerm" { |
| "zh:0fa6640b5d6c0b0fe0b7fa25cebc0091b7dda2efd83ec0347c5a50dfce0957bc", | ||
| "zh:3c8618c8a5ab07fadfcb53ecf614f7b99e25bb6c407cf0af703a6b0647d4461d", |
There was a problem hiding this comment.
Using the root resource_group_name variable with default mate-azure-task-12 and location default uksouth matches the required values from the task description (checklist #32); this resolves the earlier issue where an incorrect region was used.
| "zh:8a51dad112248d3fdfb19cdd65a182bd6a37e7fb7f1af801ab72a962813b5188", | ||
| "zh:945ff59c36de8c2128487f4ae0838655feef7c869112430ffa24ea69ff483b76", | ||
| "zh:b7306af0ecc8ad78771c17e0e911dd7a561251e4255ffe80516f0654a53f42bb", | ||
| "zh:c584de4858fc4ece330f1986f371acd235a5444e9b14ecea177779b1214539e0", | ||
| "zh:daa3675af21f8cdf330532af0b420287388f62ac57eaf0bd4c48e732d1052f35", | ||
| "zh:e76d972712b48a689f8deae41a3ad796c31d8b3798074d66c6f68e2b3b0aaa1f", | ||
| ] | ||
| } | ||
|
|
There was a problem hiding this comment.
Passing vnet_name, subnet_name, NSG, public IP name, and dns_label into the network module from root variables is good, but note that inside the module you still use defaults like subnet_name = "subnet" and allocation_method = "Static" for the public IP; make sure the module implementation enforces subnet_name = "default", public_ip_name = "linuxboxpip" and a dynamic allocation with DNS matetask + random suffix (checklist #17–#21, #38).
| hashes = [ | ||
| "h1:q/uaUTBdKgAmZESrwsoeDQff9uUA/cI/N5ZKNgVwa9c=", | ||
| "zh:161ad0bd9a75768c82f53fb6e7172a9d8be2d4889b012645a34795031aaf1bf1", | ||
| "zh:19dc9a5b17729725ccfc4f45b0500af0ee5bc6b6b160c7adb8f2bf617d2c80ea", | ||
| "zh:269eda8fe42daa7974d5a34d166c3ba9defe80cde86c01e4dadcfdf2e1f05e5f", | ||
| "zh:373f7c65566f8f2cc7f45d698654feb9d988996957e1266a69ca00c52d6d16d0", |
There was a problem hiding this comment.
The storage module wiring matches the requirement to create a storage account and a task-artifacts container (checklist #27, #29), but currently nothing uploads install-app.sh into this container; without that, the CustomScript extension’s fileUris URL will not find the script (checklist #40). Consider adding a blob upload resource or documenting the exact manual upload step that matches script_url.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "yourstorageaccount" |
There was a problem hiding this comment.
The backend configuration now uses the required storage_account_name = "yourstorageaccount" and the correct resource group/container/key, which satisfies the backend spec from the description and addresses the previous blocker around the backend settings.
| provider "azurerm" { | ||
| features { | ||
| resource_group { | ||
| prevent_deletion_if_contains_resources = false | ||
| } | ||
| } | ||
| } | ||
|
|
| module "network" { | ||
| source = "./modules/network" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = var.virtual_network_name | ||
| subnet_name = var.subnet_name | ||
| nsg_name = var.network_security_group_name | ||
| public_ip_name = var.public_ip_address_name | ||
| dns_label = var.dns_label | ||
| } |
| module "storage" { | ||
| source = "./modules/storage" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| storage_account_name = "mateazuretask12storage" | ||
| storage_container_name = "task-artifacts" | ||
| } |
There was a problem hiding this comment.
storage_account_name is hard‑coded here; that’s allowed by the task since the account name can be any value, but remember that this must actually exist (or be created here) and must match where you upload install-app.sh. Also ensure the task-artifacts container contains the script so the CustomScript extension’s URL is valid (checklist #40).
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = var.virtual_network_name | ||
| subnet_name = var.subnet_name | ||
| nsg_name = var.network_security_group_name |
There was a problem hiding this comment.
Using public_key = var.ssh_key_public is correct for treating the variable as key content, which matches root variables.tf and the requirement in checklist #39, but the variable description in this module still says it is a file path; consider updating the description to avoid confusion.
|
|
||
| module "storage" { | ||
| source = "./modules/storage" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| storage_account_name = "mateazuretask12storage" | ||
| storage_container_name = "task-artifacts" | ||
| } | ||
|
|
||
| module "compute" { |
There was a problem hiding this comment.
The VM image is correctly set to Ubuntu 22.04 (Jammy), which satisfies the requirement for Ubuntu2204 (checklist #9).
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vm_name = var.vm_name | ||
| vm_size = var.vm_size | ||
| admin_username = "azureuser" | ||
| 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 | ||
| } No newline at end of file |
There was a problem hiding this comment.
| provider "azurerm" { | ||
| features { | ||
| resource_group { |
There was a problem hiding this comment.
This output is invalid in this module because there is no azurerm_public_ip resource here; the public IP lives in the network module. You should instead expose the public IP address from the network module and then reference it at root (checklist #41).
| location = var.location | ||
| } | ||
|
|
||
| module "network" { | ||
| source = "./modules/network" | ||
| location = var.location |
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = var.virtual_network_name | ||
| subnet_name = var.subnet_name | ||
| nsg_name = var.network_security_group_name | ||
| public_ip_name = var.public_ip_address_name | ||
| dns_label = var.dns_label |
There was a problem hiding this comment.
The ssh_key_public description still says this is a path, but in root variables.tf you treat it as the actual public key content and pass it directly to public_key; update the description here to avoid misuse and to match checklist #39.
| location = var.location | ||
| } | ||
|
|
||
| module "network" { | ||
| source = "./modules/network" | ||
| location = var.location |
There was a problem hiding this comment.
The subnet default name is "subnet", but the task specifies "default" for the subnet name; while root overrides this, it’s better to align the default here to "default" to match checklist #18 and avoid surprises.
| module "storage" { | ||
| source = "./modules/storage" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| storage_account_name = "mateazuretask12storage" | ||
| storage_container_name = "task-artifacts" | ||
| } | ||
|
|
| subnet_name = var.subnet_name | ||
| nsg_name = var.network_security_group_name | ||
| public_ip_name = var.public_ip_address_name | ||
| dns_label = var.dns_label | ||
| } | ||
|
|
| provider "azurerm" { | ||
| features { | ||
| resource_group { |
There was a problem hiding this comment.
This root output references module.network.public_ip_address, but the network module does not expose a public_ip_address output, only public_ip_id. You need to add an output for azurerm_public_ip.linuxboxpip.ip_address in the network module and wire it through, per checklist #41.
| provider "azurerm" { | ||
| features { | ||
| resource_group { | ||
| prevent_deletion_if_contains_resources = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| resource "azurerm_resource_group" "rg" { | ||
| name = var.resource_group_name | ||
| location = var.location | ||
| } | ||
|
|
||
| module "network" { | ||
| source = "./modules/network" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = var.virtual_network_name | ||
| subnet_name = var.subnet_name | ||
| nsg_name = var.network_security_group_name | ||
| public_ip_name = var.public_ip_address_name | ||
| dns_label = var.dns_label | ||
| } | ||
|
|
||
| module "storage" { | ||
| source = "./modules/storage" | ||
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| storage_account_name = "mateazuretask12storage" | ||
| storage_container_name = "task-artifacts" | ||
| } | ||
|
|
||
| module "compute" { | ||
| source = "./modules/compute" | ||
| location = var.location |
There was a problem hiding this comment.
The variable definitions here now match the required defaults (names, address prefixes, location, VM size, DNS label), which aligns with checklist #32 and fixes the earlier location/variable issues.
| resource "azurerm_network_interface" "nic" { | ||
| name = "${var.vm_name}-nic" | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
|
|
||
| ip_configuration { | ||
| name = "${var.vm_name}-ipconfig" | ||
| subnet_id = var.subnet_id | ||
| private_ip_address_allocation = "Dynamic" | ||
| public_ip_address_id = var.public_ip_id | ||
| } | ||
| } | ||
|
|
||
| resource "azurerm_linux_virtual_machine" "vm" { | ||
| name = var.vm_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| size = var.vm_size | ||
| admin_username = var.admin_username | ||
| network_interface_ids = [ | ||
| azurerm_network_interface.nic.id, | ||
| ] | ||
|
|
||
| admin_ssh_key { | ||
| username = var.admin_username | ||
| public_key = var.ssh_key_public | ||
| } | ||
|
|
||
| os_disk { | ||
| caching = "ReadWrite" | ||
| storage_account_type = "Standard_LRS" | ||
| } | ||
|
|
||
| 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 compute resources (NIC, VM) correctly use the ${var.vm_name}-nic convention, dynamic private IP, and the right Ubuntu 22.04 image, which matches several checklist requirements for networking and VM image.
| resource "azurerm_virtual_machine_extension" "app" { | ||
| name = "install-app" | ||
| virtual_machine_id = azurerm_linux_virtual_machine.vm.id | ||
| publisher = "Microsoft.Azure.Extensions" | ||
| type = "CustomScript" | ||
| type_handler_version = "2.0" | ||
|
|
||
| settings = jsonencode({ | ||
| fileUris = [var.script_url] | ||
| commandToExecute = "bash install-app.sh" | ||
| }) |
There was a problem hiding this comment.
The CustomScript extension is configured to execute install-app.sh from var.script_url, which satisfies the requirement to use CustomScript for deployment; just ensure that the script is actually present at that URL in storage so this command doesn’t fail at runtime.
| resource "azurerm_network_interface" "nic" { | ||
| name = "${var.vm_name}-nic" |
There was a problem hiding this comment.
This output references azurerm_public_ip.linuxboxpip, but no such resource exists in the compute module—the public IP is created in the network module. This will cause Terraform errors and doesn’t meet checklist item #41; instead, expose the IP address from the network module and surface it at root.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| size = var.vm_size | ||
| admin_username = var.admin_username |
| } | ||
|
|
There was a problem hiding this comment.
The description says this is the path to the SSH public key, but elsewhere you treat ssh_key_public as the key content and use it directly in public_key = var.ssh_key_public. To align with the task (checklist #39), change the description to reflect that this variable holds the public key content, not a file path.
| output "public_ip_address" { | ||
| value = azurerm_public_ip.linuxboxpip.ip_address |
There was a problem hiding this comment.
The NIC naming pattern ${var.vm_name}-nic matches the requirement for the network interface name (checklist item #23), and the rest of this resource looks correct.
| output "public_ip_address" { | ||
| value = azurerm_public_ip.linuxboxpip.ip_address |
There was a problem hiding this comment.
This output references azurerm_public_ip.linuxboxpip, but there is no such resource in the compute module; the public IP is created in the network module. This will cause Terraform to fail. To satisfy checklist #41, expose the IP address from the azurerm_public_ip in the network module instead, and remove or fix this output here.
| variable "location" { | ||
| description = "The Azure region in which to create the virtual machine." | ||
| type = string | ||
| } | ||
|
|
||
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the virtual machine." | ||
| type = string | ||
| } | ||
|
|
||
| variable "vm_name" { |
| type = string | ||
| } | ||
|
|
There was a problem hiding this comment.
Using public_key = var.ssh_key_public is correct for treating ssh_key_public as the key content per the task. To avoid confusion, update the variable description in variables.tf so it no longer claims this is a file path (checklist #39).
| type = string | ||
| } | ||
|
|
||
| variable "public_ip_id" { | ||
| description = "The ID of the public IP address to associate with the network interface." | ||
| type = string |
There was a problem hiding this comment.
The VM is correctly configured with the Ubuntu 22.04 (jammy) image and the required properties, satisfying the Ubuntu2204 image requirement (checklist #9).
| variable "location" { | ||
| description = "The Azure region in which to create the virtual machine." |
There was a problem hiding this comment.
This output references azurerm_public_ip.linuxboxpip, but there is no azurerm_public_ip resource in the compute module; the public IP is defined in the network module. This will cause a Terraform error and violates checklist #41. Move the public IP output into the network module and expose it at root instead of from here.
| variable "vm_size" { | ||
| description = "The size of the virtual machine." | ||
| type = string | ||
| default = "Standard_DS1_v2" |
| variable "ssh_key_public" { | ||
| description = "The path to the SSH public key to use for authentication." | ||
| type = string |
There was a problem hiding this comment.
This description says ssh_key_public is a path, but in your VM resource you use the variable as key content and root variables.tf also defines it as content. Update this description to something like "The SSH public key content" so it aligns with checklist #39.
| resource "random_integer" "dns_suffix" { | ||
| min = 10000 | ||
| max = 99999 | ||
| } | ||
|
|
||
| resource "azurerm_virtual_network" "vnet" { | ||
| name = var.vnet_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| address_space = var.vnet_address_prefixes | ||
| } |
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefixes | ||
| } |
|
|
||
| resource "azurerm_network_security_group" "defaultnsg" { | ||
| name = var.nsg_name |
There was a problem hiding this comment.
Using public_key = var.ssh_key_public means you are treating ssh_key_public as the actual public key content, which is what the task expects (checklist #39). However, the variable description still calls it a path; update that description in variables.tf to avoid confusion.
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = "80" | ||
| source_address_prefix = "*" |
There was a problem hiding this comment.
The source image is correctly set to Ubuntu 22.04 (Jammy), which aligns with the requirement for image Ubuntu2204 (checklist #9).
| security_rule { | ||
| name = "allow-8080" | ||
| priority = 300 | ||
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = "8080" | ||
| source_address_prefix = "*" | ||
| destination_address_prefix = "*" | ||
| } |
There was a problem hiding this comment.
| resource "random_integer" "dns_suffix" { | ||
| min = 10000 |
There was a problem hiding this comment.
This output tries to read azurerm_public_ip.linuxboxpip.ip_address, but there is no azurerm_public_ip resource in the compute module; that resource is defined in the network module. This will cause a Terraform error and violates the requirement to output the IP from the correct resource/module (checklist #41). You should expose the IP address from the network module instead and surface it at root.
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefixes | ||
| } | ||
|
|
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
|
|
There was a problem hiding this comment.
The ssh_key_public description says this is the path to the key, but in main.tf you use it as the key content. To align with the root variable and checklist #39, update the description to something like “The public SSH key content to use for authentication.”
| resource "azurerm_virtual_network" "vnet" { | ||
| name = var.vnet_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| address_space = var.vnet_address_prefixes | ||
| } |
There was a problem hiding this comment.
The VNet is created using var.vnet_address_prefixes as a list, matching the required address space 10.0.0.0/16 when the defaults/inputs are set correctly (checklist #17).
| resource "azurerm_subnet" "default" { | ||
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefixes | ||
| } |
There was a problem hiding this comment.
The subnet resource uses var.subnet_name and var.subnet_address_prefixes; with the root passing default and 10.0.0.0/24, this matches the required subnet configuration (checklist #18).
| resource "azurerm_network_security_group" "defaultnsg" { | ||
| name = var.nsg_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
|
|
||
| security_rule { | ||
| name = "allow-http" | ||
| priority = 100 | ||
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = "80" | ||
| source_address_prefix = "*" | ||
| destination_address_prefix = "*" | ||
| } | ||
|
|
||
| security_rule { | ||
| name = "allow-8080" | ||
| priority = 300 | ||
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = "8080" | ||
| source_address_prefix = "*" | ||
| destination_address_prefix = "*" | ||
| } | ||
|
|
||
| security_rule { | ||
| name = "allow-ssh" | ||
| priority = 200 | ||
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = "22" | ||
| source_address_prefix = "*" | ||
| destination_address_prefix = "*" | ||
| } |
| resource "azurerm_public_ip" "linuxboxpip" { | ||
| name = var.public_ip_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
| output "subnet_id" { | ||
| description = "The ID of the created subnet." |
There was a problem hiding this comment.
This output references azurerm_public_ip.linuxboxpip, but the compute module does not define any azurerm_public_ip resource. Terraform will fail here. The public IP address should instead be exposed from the network module’s azurerm_public_ip and surfaced at root (checklist #41).
| output "public_ip_id" { | ||
| description = "The ID of the created public IP address." | ||
| value = azurerm_public_ip.linuxboxpip.id |
There was a problem hiding this comment.
The existing network outputs expose IDs, but not the actual public IP address. To satisfy checklist #41 and to fix your root output.tf, add a new output here for azurerm_public_ip.linuxboxpip.ip_address (e.g., output "public_ip_address" { value = azurerm_public_ip.linuxboxpip.ip_address }).
| output "subnet_id" { | ||
| description = "The ID of the created subnet." | ||
| value = azurerm_subnet.default.id | ||
| } | ||
|
|
||
| output "nsg_id" { | ||
| description = "The ID of the created network security group." | ||
| value = azurerm_network_security_group.defaultnsg.id | ||
| } | ||
|
|
||
| output "public_ip_id" { | ||
| description = "The ID of the created public IP address." | ||
| value = azurerm_public_ip.linuxboxpip.id |
There was a problem hiding this comment.
The storage account and container are correctly created, but there is no resource or documented step that uploads install-app.sh into the task-artifacts container. Without that, the script_url your compute module uses will 404 and the ToDo app won’t deploy, which violates checklist #3 and #40. Consider adding an azurerm_storage_blob or documenting the manual upload process.
| output "subnet_id" { | ||
| description = "The ID of the created subnet." | ||
| value = azurerm_subnet.default.id |
There was a problem hiding this comment.
This script_url output builds the correct URL for install-app.sh in the task-artifacts container, but make sure that the script is actually uploaded to that path so the CustomScript extension can download it successfully (checklist #40).
| variable "resource_group_name" { | ||
| type = string | ||
| } | ||
|
|
||
| variable "location" { | ||
| type = string | ||
| } | ||
|
|
||
| variable "vnet_name" { | ||
| description = "The name of the virtual network to create." | ||
| type = string | ||
|
|
||
| default = "vnet" |
There was a problem hiding this comment.
You expose the subnet ID correctly, but there is no output for the public IP address itself. Since the root module wants to output the VM’s public IP and checklist #41 requires using the azurerm_public_ip resource’s ip_address, consider adding an additional output (e.g. public_ip_address = azurerm_public_ip.linuxboxpip.ip_address).
| variable "subnet_name" { | ||
| description = "The name of the subnet to create." | ||
| type = string | ||
|
|
||
| default = "subnet" |
There was a problem hiding this comment.
The subnet_name default is "subnet", but the task specifies a subnet named "default". Root currently overrides this, so it works, but updating the default here would align the module with the specification and avoid surprises.
| variable "public_ip_name" { | ||
| description = "The name of the public IP address to create." | ||
| type = string | ||
| default = "publicip" |
There was a problem hiding this comment.
The public_ip_name default is "publicip", whereas the task requires the public IP to be named "linuxboxpip". Root overrides this, but aligning the default here to "linuxboxpip" will make the module match the task description more closely.
| variable "resource_group_name" { | ||
| type = string | ||
| } | ||
|
|
||
| variable "location" { | ||
| type = string | ||
| } | ||
|
|
||
| variable "vnet_name" { | ||
| description = "The name of the virtual network to create." | ||
| type = string | ||
|
|
||
| default = "vnet" |
There was a problem hiding this comment.
This module correctly creates the storage account and task-artifacts container, but it doesn’t actually upload install-app.sh. Since the compute module’s CustomScript extension relies on the script being available at script_url, you either need a resource to upload the file or clear instructions elsewhere to ensure the script exists at that URL.
| variable "resource_group_name" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The script_url output builds the correct URL for install-app.sh in the task-artifacts container. Just make sure that the file is actually present there, otherwise the CustomScript extension will fail to download it.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location | ||
| account_tier = "Standard" | ||
| account_replication_type = "LRS" | ||
| } | ||
|
|
||
| resource "azurerm_storage_container" "container" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.storage.name | ||
| container_access_type = "blob" | ||
| } |
There was a problem hiding this comment.
You correctly output the subnet ID and NSG ID, but there is no output for the public IP address itself. To meet checklist #41 and support your root output.tf, add an output like output "public_ip_address" { value = azurerm_public_ip.linuxboxpip.ip_address } here.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location | ||
| account_tier = "Standard" | ||
| account_replication_type = "LRS" | ||
| } | ||
|
|
||
| resource "azurerm_storage_container" "container" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.storage.name | ||
| container_access_type = "blob" | ||
| } |
There was a problem hiding this comment.
The storage account and container definitions satisfy the requirement to create a storage account and a task-artifacts container for artifacts (checklist #27 and #29). However, note that nothing here uploads install-app.sh into this container; that still needs to be handled elsewhere to satisfy checklist #40.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name | ||
| resource_group_name = var.resource_group_name |
There was a problem hiding this comment.
| output "script_url" { | ||
| value = "https://${azurerm_storage_account.storage.name}.blob.core.windows.net/task-artifacts/install-app.sh" | ||
| description = "The URL of the install script in Azure Blob Storage." |
There was a problem hiding this comment.
This URL is correctly constructed for the task-artifacts/install-app.sh blob, but Terraform does not upload that script by itself. To avoid the CustomScript extension failing with a 404, either add a resource to upload install-app.sh to this container or clearly document the manual upload step that matches this exact path (checklist #40).
| variable "storage_account_name" { | ||
| description = "The name of the storage account." |
There was a problem hiding this comment.
This root output expects module.network.public_ip_address, but the network module currently only outputs public_ip_id. You need to add a public_ip_address output in modules/network/outputs.tf that returns azurerm_public_ip.linuxboxpip.ip_address so this reference is valid (checklist #41).
| output "public_ip" { | ||
| value = module.network.public_ip_address |
There was a problem hiding this comment.
This root output assumes the network module exposes public_ip_address. Currently, modules/network/outputs.tf doesn’t define that output; you need to add output "public_ip_address" { value = azurerm_public_ip.linuxboxpip.ip_address } in the network module to satisfy checklist #41 and make this reference valid.
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the resources." |
There was a problem hiding this comment.
This output assumes that the network module defines public_ip_address, but modules/network/outputs.tf currently only defines subnet_id, nsg_id, and public_ip_id. You need to add a public_ip_address output in the network module (using azurerm_public_ip.linuxboxpip.ip_address) to avoid a Terraform error and satisfy checklist #41.
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the resources." | ||
| type = string | ||
| default = "mate-azure-task-12" | ||
| } | ||
|
|
||
| variable "location" { | ||
| description = "The Azure region in which to create the resources." | ||
| type = string | ||
| default = "uksouth" | ||
| } | ||
|
|
||
| variable "virtual_network_name" { | ||
| description = "The name of the virtual network." | ||
| type = string | ||
| default = "vnet" | ||
| } | ||
|
|
||
| variable "vnet_address_prefix" { | ||
| description = "The address prefix for the virtual network." | ||
| type = string | ||
| default = "10.0.0.0/16" | ||
| } | ||
|
|
||
| variable "subnet_name" { | ||
| description = "The name of the subnet." | ||
| type = string | ||
| default = "default" | ||
| } | ||
|
|
||
| variable "subnet_address_prefix" { | ||
| description = "The address prefix for the subnet." | ||
| type = string | ||
| default = "10.0.0.0/24" | ||
| } | ||
|
|
||
| variable "network_security_group_name" { | ||
| description = "The name of the network security group." | ||
| type = string | ||
| default = "defaultnsg" | ||
| } | ||
|
|
||
| variable "public_ip_address_name" { | ||
| description = "The name of the public IP address." | ||
| type = string | ||
| default = "linuxboxpip" | ||
| } | ||
|
|
||
| variable "vm_name" { | ||
| description = "The name of the virtual machine." | ||
| type = string | ||
| default = "matebox" | ||
| } | ||
|
|
||
| variable "vm_size" { | ||
| description = "The size of the virtual machine." | ||
| type = string | ||
| default = "Standard_B1s" | ||
| } | ||
|
|
||
| variable "ssh_key_public" { | ||
| description = "The public SSH key content." | ||
| type = string | ||
| } | ||
|
|
||
| variable "dns_label" { | ||
| description = "The DNS label for the public IP." | ||
| type = string | ||
| default = "matetask" |
There was a problem hiding this comment.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine it further. You’ve done a strong job structuring the project into network, compute, and storage modules, wiring them together from main.tf, using the correct Ubuntu 22.04 image, naming the NIC as ${var.vm_name}-nic, and configuring a CustomScript extension to run install-app.sh from Blob Storage. The main functional gaps to address next are: (1) your public IP in modules/network/main.tf still uses allocation_method = "Static" instead of the required dynamic allocation, (2) the compute module’s vm_size default in modules/compute/variables.tf is "Standard_DS1_v2" instead of "Standard_B1s", (3) the ssh_key_public description in the compute variables still says it is a path instead of public key content, and (4) there is no azurerm_storage_blob (or documented manual step) that actually uploads install-app.sh into the task-artifacts container, so the VM extension’s fileUris URL would 404. In addition, your compute output public_ip_vm references azurerm_linux_virtual_machine.vm.public_ip_address, which is not a valid attribute on that resource; rely on the network module’s public_ip_address output instead or change this to a valid VM attribute. Once you switch the public IP to dynamic, fix the default VM size, correct the SSH key description, and ensure install-app.sh is really present in Blob Storage at the URL you construct, your configuration will both meet the formal requirements and work reliably end to end. Overall, you’re very close—your module layout, backend configuration, and most resource definitions are correct, so these refinements will be great practice in aligning implementation details with written requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| "zh:78d5eefdd9e494defcb3c68d282b8f96630502cac21d1ea161f53cfe9bb483b3", | ||
| "zh:8a51dad112248d3fdfb19cdd65a182bd6a37e7fb7f1af801ab72a962813b5188", | ||
| "zh:945ff59c36de8c2128487f4ae0838655feef7c869112430ffa24ea69ff483b76", | ||
| "zh:b7306af0ecc8ad78771c17e0e911dd7a561251e4255ffe80516f0654a53f42bb", |
There was a problem hiding this comment.
This script is cloned from GitHub at runtime, but your CustomScript extension is configured to download install-app.sh from Azure Blob Storage. Without a Terraform resource or documented step that uploads this file into the task-artifacts container at the URL referenced in script_url, the extension will fail to download and execute it, blocking the required ToDo app deployment.
| mkdir /app | ||
| # make sure to uncomment the line bellow and update the link with your GitHub username | ||
| # git clone https://github.com/<your-gh-username>/azure_task_12_deploy_app_with_vm_extention.git | ||
| git clone https://github.com/bohn13/devops_todolist_terraform_task.git |
There was a problem hiding this comment.
The comment still suggests editing this repo URL with your own GitHub username, but it’s hardcoded to a specific user now; either adjust the comment or parameterize the URL so it matches how you expect students/consumers to use this script.
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| vnet_name = var.virtual_network_name | ||
| subnet_name = var.subnet_name |
There was a problem hiding this comment.
vm_size default is still "Standard_DS1_v2", but the task requires the default size to be "Standard_B1s". Update this default to match the specification so the module behaves correctly even when a value isn’t passed.
| location = var.location | ||
| resource_group_name = azurerm_resource_group.rg.name |
There was a problem hiding this comment.
This description says it’s a path to the SSH public key, but in your root module you pass the public key content and use it directly in admin_ssh_key.public_key. Update the text to clarify it expects public key content, not a filesystem path, to align with the task requirements and actual usage.
| provider "azurerm" { | ||
| features { |
There was a problem hiding this comment.
azurerm_linux_virtual_machine doesn’t expose a public_ip_address attribute; the public IP is managed as a separate resource in the network module. This output will cause a Terraform error. Consider removing this output or sourcing the IP from the network module instead.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| size = var.vm_size | ||
| admin_username = var.admin_username |
There was a problem hiding this comment.
This default value for vm_size does not match the task requirement. The description requires the VM size to be Standard_B1s by default; update this default to "Standard_B1s" so the module behavior aligns with the specification.
| } | ||
|
|
There was a problem hiding this comment.
The description claims this is a path to the SSH public key, but you are actually passing and using the public key content (not a file path). Update the description to reflect that it should contain the public key string, as required by the task.
| resource "azurerm_network_interface" "nic" { | ||
| name = "${var.vm_name}-nic" |
There was a problem hiding this comment.
azurerm_linux_virtual_machine does not expose a public_ip_address attribute, and in your design the public IP resource lives in the network module. This output will fail; consider removing it or outputting a valid attribute (e.g., VM ID) and rely on the network module’s public_ip_address output instead.
| output "public_ip_vm" { | ||
| value = azurerm_linux_virtual_machine.vm.public_ip_address |
There was a problem hiding this comment.
azurerm_linux_virtual_machine typically does not expose a direct public_ip_address attribute; the public IP is managed by azurerm_public_ip in the network module. This output may not work as intended—consider either removing it or wiring the public IP output from the network module at the root level instead.
| variable "location" { | ||
| description = "The Azure region in which to create the virtual machine." |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.vm.public_ip_address, but the Linux VM resource does not expose a simple public_ip_address attribute. Public IP information is tied to the NIC/public IP resources, so this output is likely invalid and will either fail or always be null. Consider removing it or wiring through the actual public IP from the network module instead.
| variable "vm_size" { | ||
| description = "The size of the virtual machine." | ||
| type = string | ||
| default = "Standard_DS1_v2" |
There was a problem hiding this comment.
The default VM size here is "Standard_DS1_v2", but the task requires the default VM size to be "Standard_B1s". Update this default to align with the specification so that the module behaves correctly when vm_size is not explicitly overridden.
| variable "ssh_key_public" { | ||
| description = "The path to the SSH public key to use for authentication." |
There was a problem hiding this comment.
This description says this is a path to an SSH public key, but you actually pass the key content and use it directly in admin_ssh_key.public_key. The task expects public key content, so update the description to reflect that (e.g., "The SSH public key content to use for authentication").
| resource "azurerm_public_ip" "linuxboxpip" { | ||
| name = var.public_ip_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
The task requires the public IP to use a dynamic allocation method. Here it is still configured as "Static" with a Standard SKU, which conflicts with the specification. Update allocation_method to "Dynamic" (and adjust SKU if needed) to comply with the requirements.
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = var.subnet_address_prefixes |
There was a problem hiding this comment.
The network module outputs are wired correctly and expose the public IP address, which your root module uses. This is the right place to obtain the VM’s public IP for verification.
| } | ||
|
|
||
| resource "azurerm_network_security_group" "defaultnsg" { | ||
| name = var.nsg_name | ||
| location = var.location |
There was a problem hiding this comment.
The default subnet_name is set to "subnet", while the task spec and your root variables use "default". Since you pass var.subnet_name from the root this won’t break anything, but for consistency with the task’s naming it would be better to make the default "default".
| resource_group_name = var.resource_group_name | ||
| address_space = var.vnet_address_prefixes | ||
| } | ||
|
|
There was a problem hiding this comment.
The storage module only creates the storage account and container; there is no resource here to upload install-app.sh into this container. Because your CustomScript extension downloads the script from this container, you need either a Terraform blob resource or clear documentation of a manual upload step to avoid a 404 when the VM extension runs.
| output "subnet_id" { | ||
| description = "The ID of the created subnet." |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.vm.public_ip_address, but that attribute does not exist on the VM resource; the public IP is managed by a separate azurerm_public_ip in the network module. This output will likely cause an error or be null; consider removing it and relying on the public_ip_address output from the network module instead.
| } | ||
|
|
||
| output "public_ip_id" { | ||
| description = "The ID of the created public IP address." | ||
| value = azurerm_public_ip.linuxboxpip.id |
There was a problem hiding this comment.
The storage account and container are created, but there is no resource here that uploads install-app.sh into the task-artifacts container. Since your CustomScript extension downloads the script from Blob Storage, you need either a blob resource to upload the script or clear documentation for a manual upload step; otherwise, the extension will 404 when fetching the script.
| variable "vnet_name" { | ||
| description = "The name of the virtual network to create." | ||
| type = string | ||
|
|
||
| default = "vnet" |
There was a problem hiding this comment.
The storage module only creates the account and container. There is no resource here (or in other shown files) that uploads install-app.sh into the task-artifacts container, even though the compute module’s CustomScript expects to download it from Blob Storage. Without an upload resource or clearly documented manual upload step, the extension’s fileUris will 404 and the app deployment will fail.
| variable "vnet_address_prefixes" { | ||
| description = "The address prefixes for the virtual network." | ||
| type = list(string) | ||
|
|
There was a problem hiding this comment.
The default for vm_size is "Standard_DS1_v2", but the task explicitly requires the default to be "Standard_B1s". Update this default to comply with the specification and keep module behavior consistent with the root variables.
| default = "subnet" | ||
| } |
There was a problem hiding this comment.
This description says this is the path to the SSH public key, but in your root module you pass the public key content and use it directly in admin_ssh_key.public_key. Change the description to indicate it expects the public key content string, to align with the task requirements and actual usage.
| variable "resource_group_name" { | ||
| type = string |
There was a problem hiding this comment.
azurerm_linux_virtual_machine does not expose a public_ip_address attribute; the public IP is managed by the azurerm_public_ip resource in the network module. This output will not work as written. Consider removing it or instead relying on the public_ip_address output from the network module at the root level.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.vm.public_ip_address, but that attribute does not exist on the VM resource. The public IP is exposed via the azurerm_public_ip in the network module (and you already have a public_ip_address output there). Consider removing this output or replacing it with a valid VM attribute.
| resource "azurerm_storage_container" "container" { | ||
| name = var.storage_container_name | ||
| storage_account_name = azurerm_storage_account.storage.name | ||
| container_access_type = "blob" | ||
| } |
There was a problem hiding this comment.
You correctly create a storage account and task-artifacts container, but there is no resource here that uploads install-app.sh into this container. Because the CustomScript extension downloads the script from Blob Storage, you need either an azurerm_storage_blob (or similar) to upload the script or clear documentation for a manual upload step; otherwise, the extension will 404 when trying to fetch the script.
| output "script_url" { | ||
| value = "https://${azurerm_storage_account.storage.name}.blob.core.windows.net/task-artifacts/install-app.sh" |
There was a problem hiding this comment.
azurerm_linux_virtual_machine does not expose a public_ip_address attribute; the public IP is represented by a separate azurerm_public_ip in the network module. This output will likely fail or always be null. Since you already expose public_ip_address from the network module and root, consider removing this output or changing it to a valid VM attribute.
| output "script_url" { | ||
| value = "https://${azurerm_storage_account.storage.name}.blob.core.windows.net/task-artifacts/install-app.sh" |
There was a problem hiding this comment.
You construct a script_url assuming install-app.sh exists in the task-artifacts container, but there is no Terraform resource that uploads the script file into this container and no documented manual step. Without uploading install-app.sh to this path, the CustomScript extension’s fileUris will 404 and the ToDo app won’t deploy.
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the storage account." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
vm_size still defaults to "Standard_DS1_v2", which contradicts the task requirement that the default VM size be "Standard_B1s". Update this default to keep the compute module consistent with the description and your root variables.
| variable "storage_account_name" { | ||
| description = "The name of the storage account." |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.vm.public_ip_address, but the VM resource does not expose a public_ip_address attribute; the public IP is managed by the azurerm_public_ip in the network module. Since you already output public_ip_address from the network module, consider removing this output or changing it to a valid VM attribute instead.
| variable "storage_account_name" { | ||
| description = "The name of the storage account." |
There was a problem hiding this comment.
You correctly construct a script_url to install-app.sh in Blob Storage, but there is no Terraform resource here (or elsewhere) that uploads the install-app.sh file into the task-artifacts container. Without a blob upload resource or a clearly documented manual upload step, the VM extension’s fileUris will 404 when trying to download the script, preventing the app from being deployed.
| output "public_ip" { | ||
| value = module.network.public_ip_address |
There was a problem hiding this comment.
This script_url assumes that install-app.sh is present in the task-artifacts container, but there is no Terraform resource (e.g., azurerm_storage_blob) or documented manual step that actually uploads the script to this location. Without that, the CustomScript extension will 404 when trying to download the script, and the ToDo app deployment will fail.
| default = "vnet" | ||
| } | ||
|
|
||
| variable "vnet_address_prefix" { |
There was a problem hiding this comment.
The VM size default here is "Standard_DS1_v2", which contradicts the requirement that the default VM size must be "Standard_B1s". Update this default to "Standard_B1s" so the compute module aligns with the task even when vm_size isn’t overridden.
| type = string | ||
| default = "default" |
There was a problem hiding this comment.
This variable is documented as a path to the SSH public key, but in practice you pass and use the public key content from the root module (admin_ssh_key.public_key). Update the description to clearly state it expects public key content to align with both usage and the task description.
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the resources." |
There was a problem hiding this comment.
This output references azurerm_linux_virtual_machine.vm.public_ip_address, but that attribute doesn’t exist on the VM resource; the public IP is represented by the azurerm_public_ip in the network module. Since you already output the public IP via the network module and root output.tf, consider removing this output or changing it to a valid attribute (like VM ID or name).
| variable "resource_group_name" { | ||
| description = "The name of the resource group in which to create the resources." |
There was a problem hiding this comment.
You construct a script_url that assumes install-app.sh already exists in the task-artifacts container, but there is no Terraform resource here that uploads the script, nor are there documented manual steps in this code. Without uploading the script to this exact path, the CustomScript extension’s fileUris will 404 and the ToDo app won’t deploy. Consider adding an azurerm_storage_blob or documenting a manual upload process that matches this URL.
2 participants
No description provided.