Deploy TodoApp with Terraform#54
Conversation
Updated the installation script to clone the repository and copy app files from the cloned directory.
Updated installation script to clone a different repository and set up a Python virtual environment.
Updated the start script to activate virtual environment and adjust paths.
Add Terraform Azure infra modules and update startup scripts
…date naming conventions for clarity
There was a problem hiding this comment.
Pull request overview
This PR adds Terraform infrastructure to deploy the TodoApp on Azure using a modular setup (network/compute/storage) and updates the VM bootstrap scripts + CI workflow to validate the Terraform configuration.
Changes:
- Add root Terraform configuration and three modules (network, compute, storage) to provision Azure resources for the app.
- Add environment configuration files (
terraform.tfvars,backend.tf) and variable definitions. - Update VM provisioning/runtime scripts (
install-app.sh,app/start.sh) and adjust the GitHub Actions Terraform validation workflow to run from theterraform/directory.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| terraform/main.tf | Root module wiring for RG + network/compute/storage modules and NIC↔NSG association |
| terraform/backend.tf | AzureRM backend configuration for remote state |
| terraform/terraform.tfvars | Concrete variable values for a target environment |
| terraform/network_variables.tf | Root-level network variable declarations |
| terraform/compute_variables.tf | Root-level compute variable declarations |
| terraform/storage_variables.tf | Root-level storage variable declarations |
| terraform/outputs.tf | Placeholder root outputs file (currently empty) |
| terraform/modules/network/main.tf | VNet/Subnet/NSG/Public IP provisioning logic |
| terraform/modules/network/variables.tf | Network module inputs |
| terraform/modules/network/outputs.tf | Network module outputs (subnet, nsg, public ip) |
| terraform/modules/compute/main.tf | NIC, Linux VM, and CustomScript extension provisioning |
| terraform/modules/compute/variables.tf | Compute module inputs |
| terraform/modules/compute/outputs.tf | Compute module outputs (NIC/VM/extension IDs) |
| terraform/modules/storage/main.tf | Storage account + container resources |
| terraform/modules/storage/variables.tf | Storage module inputs |
| terraform/modules/storage/outputs.tf | Storage module outputs (ids/endpoints) |
| install-app.sh | VM bootstrap script to install dependencies and set up systemd service |
| app/start.sh | App startup script updated to use a venv and exec the server |
| .github/workflows/terraform-validate.yml | Run terraform checks from terraform/ via workflow defaults |
| terraform/tfplan | Committed Terraform plan artifact (binary) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name = var.public_ip_address_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" |
There was a problem hiding this comment.
sku = "Standard" is not compatible with allocation_method = "Dynamic" for azurerm_public_ip (Standard Public IPs require Static allocation). Update either the SKU (e.g., Basic) or set allocation_method to Static so applies don't fail.
| allocation_method = "Dynamic" | |
| allocation_method = "Static" |
| protected_settings = jsonencode({ | ||
| fileUris = [ | ||
| "https://raw.githubusercontent.com/whatislavx/devops_todolist_terraform_task/main/install-app.sh" | ||
| ], | ||
|
|
||
| commandToExecute = "chmod +x install-app.sh && ./install-app.sh" | ||
| }) |
There was a problem hiding this comment.
The module input script_file is passed in from the root module but is not used here; instead the extension downloads install-app.sh from the main branch of GitHub. This means deployments from this branch won’t use the code under review, and it makes the module less reusable. Use var.script_file (e.g., as a URL) or embed the local script content (e.g., base64-encode file(var.script_file)) so the extension runs the intended script.
| /app/venv/bin/python -m pip install --upgrade pip | ||
| /app/venv/bin/python -m pip install -r /app/requirements.txt | ||
| /app/venv/bin/python /app/manage.py migrate |
There was a problem hiding this comment.
This script upgrades pip and reinstalls dependencies on every service start. That makes restarts slow and introduces a runtime dependency on PyPI/network availability. Install dependencies once during provisioning (e.g., in install-app.sh) and keep start.sh focused on launching the app process.
| backend "azurerm" { | ||
| 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 configuration is hardcoded to a specific resource group and a placeholder storage account name. This will make terraform init fail for anyone who doesn’t have that exact backend provisioned. Consider documenting a bootstrap step / using partial backend config (supplied via -backend-config) or providing this as an example file rather than an always-on backend.
| backend "azurerm" { | |
| resource_group_name = "mate-azure-task-12" | |
| storage_account_name = "yourstorageaccount" | |
| container_name = "tfstate" | |
| key = "terraform.tfstate" | |
| # Backend configuration for Azure Storage. | |
| # | |
| # The specific backend settings (resource group, storage account, container, key) | |
| # are intentionally not hardcoded here so that `terraform init` does not fail for | |
| # users who do not have an identical Azure backend provisioned. | |
| # | |
| # Supply these values via `-backend-config` flags or a separate backend config | |
| # file when running `terraform init`, for example: | |
| # | |
| # terraform init \ | |
| # -backend-config="resource_group_name=YOUR_RESOURCE_GROUP" \ | |
| # -backend-config="storage_account_name=YOUR_STORAGE_ACCOUNT" \ | |
| # -backend-config="container_name=YOUR_CONTAINER" \ | |
| # -backend-config="key=YOUR_TFSTATE_KEY" | |
| # | |
| backend "azurerm" { |
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" | ||
| destination_port_range = security_rule.value.destination_port_range | ||
| source_address_prefix = "*" | ||
| destination_address_prefix = "*" | ||
| description = security_rule.value.description |
There was a problem hiding this comment.
Inbound NSG rules are allowing SSH (22) and the app port (8080) from source_address_prefix = "*" (the entire internet). This is a security risk; make the allowed source CIDRs configurable (e.g., allowed_ssh_cidrs, allowed_app_cidrs) and default to something safer than * (or at least restrict SSH).
| # create a service for the app via systemctl and start the app | ||
| mv /app/todoapp.service /etc/systemd/system/ | ||
| chmod +x start.sh | ||
| cp /tmp/todolist/todoapp.service /etc/systemd/system/todoapp.service |
There was a problem hiding this comment.
The service file is copied from /tmp/todolist/todoapp.service, but in this repository the unit file lives under app/todoapp.service (and you copy app/* into /app). As written, this cp will fail and the service won’t be installed. Copy from /tmp/todolist/app/todoapp.service or from /app/todoapp.service after the previous cp -r.
| cp /tmp/todolist/todoapp.service /etc/systemd/system/todoapp.service | |
| cp /app/todoapp.service /etc/systemd/system/todoapp.service |
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is effectively empty (only whitespace). Consider removing it or adding the intended root module outputs (e.g., public IP/FQDN) so the repository doesn’t carry placeholder files.
| // outputs.tf | |
| // This module currently has no outputs defined. | |
| // Add root module outputs here (e.g., public IPs, FQDNs, IDs) when needed. |
| git clone https://github.com/whatislavx/devops_todolist_terraform_task.git /tmp/todolist | ||
| cp -r /tmp/todolist/app/* /app |
There was a problem hiding this comment.
This script clones the repository from the main branch (git clone .../main/...). When deploying changes from another branch/commit, the VM will still pull main, so the deployed app may not match the Terraform/app code being applied. Consider downloading the application artifacts tied to the same commit (e.g., via the VM extension fileUris), or make the repo URL/ref configurable via Terraform variables.
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 learning, even though there are still a few issues to address. You’ve done well setting up a modular Terraform structure (network/compute/storage), wiring the modules together in terraform/main.tf, defining an azurerm backend, and adding a CustomScript VM extension plus install scripts for the app. The most important fixes to focus on next are: aligning SSH key variables so everything consistently uses a content variable named ssh_key_public (instead of path-based or differently named variables), ensuring the public IP uses a compatible combination of sku and allocation_method (e.g., Basic + Dynamic), and making sure the VM extension actually runs your install-app.sh and that this script installs Python dependencies into the venv and sets up the service correctly. You should also move terraform.tfvars to the repository root, add meaningful outputs to outputs.tf (at least VM public IP and FQDN), and tighten the DNS label randomness so it’s matetask plus digits only, to fully meet the checklist expectations. Overall, you’re very close to a complete solution; with these targeted corrections, your deployment will be much more robust and easier to verify—nice work getting this far, and keep iterating on these points.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -1,5 +1,9 @@ | |||
| #! /bin/bash | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
If this script is the one the VM extension should run, the task requires a file named install-app.sh. Either rename this file to install-app.sh or ensure the VM extension is configured to execute start.sh. Also consider using a more portable shebang (#!/usr/bin/env bash) and add set -e immediately after the shebang so the script exits on any error (helps automation).
| python3 manage.py runserver 0.0.0.0:8080 No newline at end of file | ||
| source /app/venv/bin/activate | ||
|
|
||
| /app/venv/bin/python -m pip install --upgrade pip |
There was a problem hiding this comment.
This command is fine, but consider adding --no-cache-dir to pip to reduce layer size and avoid caching issues when running in constrained environments: python -m pip install --upgrade pip --no-cache-dir.
| source /app/venv/bin/activate | ||
|
|
||
| /app/venv/bin/python -m pip install --upgrade pip | ||
| /app/venv/bin/python -m pip install -r /app/requirements.txt |
There was a problem hiding this comment.
Installing requirements from /app/requirements.txt is correct; ensure that requirements.txt is present at /app/requirements.txt in the VM image or that previous steps place it there. Consider --no-cache-dir here as well for reliability and space savings.
|
|
||
| /app/venv/bin/python -m pip install --upgrade pip | ||
| /app/venv/bin/python -m pip install -r /app/requirements.txt | ||
| /app/venv/bin/python /app/manage.py migrate |
There was a problem hiding this comment.
Running migrations is required, but in automated deployments prefer a non-interactive flag to avoid blocking: python /app/manage.py migrate --noinput. Confirm this is what you want in CI/automation contexts.
| /app/venv/bin/python -m pip install -r /app/requirements.txt | ||
| /app/venv/bin/python /app/manage.py migrate | ||
|
|
||
| exec /app/venv/bin/python /app/manage.py runserver 0.0.0.0:8080 |
There was a problem hiding this comment.
Using Django's runserver is acceptable for demos, but it's not production-ready. For a more robust deployment consider running a WSGI server such as gunicorn behind a reverse proxy or a supervised service so the app survives reboots and crashes.
| defaults: | ||
| run: | ||
| working-directory: terraform |
There was a problem hiding this comment.
The job default sets run.working-directory to terraform. The task expects top-level files like main.tf and modules/... (not under a terraform/ dir). If your Terraform files are at the repository root, remove this default or change it to . so the grep checks and terraform commands operate in the correct directory.
| apt-get install -yq python3 python3-venv python3-pip git | ||
|
|
||
| mkdir -p /app | ||
| python3 -m venv /app/venv |
There was a problem hiding this comment.
The script creates a Python virtual environment but never installs the application's Python dependencies into it (no pip install -r requirements.txt or equivalent). This will likely make the app fail at runtime. Consider activating the venv and installing requirements from the cloned repo (for example: source /app/venv/bin/activate && pip install -r /tmp/todolist/app/requirements.txt).
| # when running the script as VM extention. | ||
| apt-get update -yq | ||
| apt-get install python3-pip -yq | ||
| apt-get install -yq python3 python3-venv python3-pip git |
There was a problem hiding this comment.
You run apt-get install here but the earlier apt-get update line used an invalid flag (see review summary). Ensure you run apt-get update (without -y) before this, and keep apt-get install -y ... here. Also consider using DEBIAN_FRONTEND=noninteractive apt-get -y install ... for noninteractive provisioning.
| mkdir -p /app | ||
| python3 -m venv /app/venv | ||
|
|
||
| git clone https://github.com/whatislavx/devops_todolist_terraform_task.git /tmp/todolist |
There was a problem hiding this comment.
Cloning directly into /tmp/todolist is OK, but if this script runs multiple times the directory may already exist and git clone will fail. Consider using git clone ... || (cd /tmp/todolist && git pull) or remove the directory first to make this step idempotent.
| python3 -m venv /app/venv | ||
|
|
||
| git clone https://github.com/whatislavx/devops_todolist_terraform_task.git /tmp/todolist | ||
| cp -r /tmp/todolist/app/* /app |
There was a problem hiding this comment.
Copying app/* into /app is fine but will fail if the app subdirectory does not exist in the repo or contains dotfiles. Also preserve executable bits where needed (or set them explicitly). Confirm the repo structure and consider rsync -a for safer copying.
|
|
||
| # create a service for the app via systemctl and start the app | ||
| mv /app/todoapp.service /etc/systemd/system/ | ||
| chmod +x start.sh |
There was a problem hiding this comment.
You chmod +x start.sh but do not verify the file exists. If the copy failed earlier this will error and abort the script. Consider checking file existence or combine making executable when copying (install -m 0755 /tmp/todolist/app/start.sh /app/start.sh).
| # create a service for the app via systemctl and start the app | ||
| mv /app/todoapp.service /etc/systemd/system/ | ||
| chmod +x start.sh | ||
| cp /tmp/todolist/todoapp.service /etc/systemd/system/todoapp.service |
There was a problem hiding this comment.
Copying the systemd unit into /etc/systemd/system is correct; ensure the unit file references the venv Python interpreter (e.g. ExecStart=/app/venv/bin/python /app/app.py or that start.sh activates the venv). Otherwise the service will run system Python and may not find installed packages.
| systemctl daemon-reload | ||
| systemctl start todoapp | ||
| systemctl enable todoapp | ||
| systemctl start todoapp |
There was a problem hiding this comment.
You enable and start the service which is good, but there is no verification step (e.g. systemctl status or journalctl -u todoapp --no-pager -n 200) nor a health check for the HTTP endpoint. Consider adding a simple check to confirm the web server started successfully and fail early if not.
| @@ -0,0 +1,8 @@ | |||
| terraform { | |||
| backend "azurerm" { | |||
There was a problem hiding this comment.
This declares the azurerm backend block — make sure the AzureRM provider and authentication (CLI, SP, or Managed Identity) are configured in the root module or environment before running terraform init, otherwise backend initialization will fail.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "yourstorageaccount" |
There was a problem hiding this comment.
The backend references the storage account yourstorageaccount and container tfstate. Per the task requirements, the storage account and container must exist (either created in main.tf or managed separately) before terraform init. If yourstorageaccount is a placeholder, replace it or ensure that resource is created.
|
|
||
| output "nic_private_ip" { | ||
| description = "Private IP address of the NIC" | ||
| value = azurerm_network_interface.nic.private_ip_address |
There was a problem hiding this comment.
Referencing the NIC private IP as azurerm_network_interface.nic.private_ip_address may be invalid depending on the provider version. Use the ip_configuration block index to access the computed private IP, e.g. azurerm_network_interface.nic.ip_configuration[0].private_ip_address, to ensure compatibility.
| # VM | ||
| output "vm_id" { | ||
| description = "ID of the Virtual Machine" | ||
| value = azurerm_linux_virtual_machine.vm.id |
There was a problem hiding this comment.
This output assumes the VM resource is named vm in the module (azurerm_linux_virtual_machine.vm). Confirm the actual resource name in compute/main.tf matches this; otherwise the output will fail with an unknown resource error.
| # VM Extension | ||
| output "custom_script_extension_id" { | ||
| description = "ID of the Custom Script VM Extension" | ||
| value = azurerm_virtual_machine_extension.custom_script.id |
There was a problem hiding this comment.
This output assumes the VM extension resource is named custom_script (azurerm_virtual_machine_extension.custom_script). Confirm the resource name matches the extension declared in the module. Also ensure that the extension is indeed the CustomScript extension that runs install-app.sh so this output is meaningful.
| output "custom_script_extension_id" { | ||
| description = "ID of the Custom Script VM Extension" | ||
| value = azurerm_virtual_machine_extension.custom_script.id | ||
| } |
There was a problem hiding this comment.
Consider adding an output for the VM public IP or its DNS label (or referencing the public IP output from the network module). The overall task requires accessing the VM via public IP/DNS to verify the app — having it as an output makes verification easier.
| } | ||
|
|
||
| resource "azurerm_linux_virtual_machine" "vm" { | ||
| name = var.vm_name |
There was a problem hiding this comment.
The VM is named using var.vm_name. The task requires the VM name to be matebox. Ensure var.vm_name (in your variables.tf/terraform.tfvars) is set to matebox so the final VM name matches the requirement.
| name = var.vm_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location | ||
| size = var.vm_size |
There was a problem hiding this comment.
The VM size is var.vm_size. The task requires vm_size to be Standard_B1s. Ensure var.vm_size is set to Standard_B1s in your variables.tf/terraform.tfvars so the created VM uses the required size.
|
|
||
| admin_ssh_key { | ||
| username = var.admin_username | ||
| public_key = var.ssh_public_key |
There was a problem hiding this comment.
This uses var.ssh_public_key but the task's variables list requires ssh_key_public. This name mismatch will cause Terraform to fail to find the expected variable. Rename this usage to var.ssh_key_public or update your variables to include ssh_public_key as appropriate.
| type = "CustomScript" | ||
| type_handler_version = "2.0" | ||
|
|
||
| protected_settings = jsonencode({ |
There was a problem hiding this comment.
You placed fileUris and commandToExecute inside protected_settings using jsonencode. Typically non-secret settings like fileUris and commandToExecute belong in settings (as a map) for the CustomScript extension. Verify the provider version/schema and consider using settings = jsonencode({...}) (or a map) instead to avoid runtime issues.
|
|
||
| protected_settings = jsonencode({ | ||
| fileUris = [ | ||
| "https://raw.githubusercontent.com/whatislavx/devops_todolist_terraform_task/main/install-app.sh" |
There was a problem hiding this comment.
The file URI points to https://raw.githubusercontent.com/whatislavx/devops_todolist_terraform_task/main/install-app.sh. Make sure this is the correct install-app.sh you intend to run (and accessible). The task expects the extension to execute install-app.sh from your deployment artifacts — referencing an external repo may not satisfy the requirement if it's not your script or if it moves.
| vm_name = var.vm_name | ||
| vm_size = var.vm_size | ||
| admin_username = var.admin_username | ||
| ssh_public_key = file(var.ssh_key_public_path) |
There was a problem hiding this comment.
The compute module is given ssh_public_key = file(var.ssh_key_public_path). The task requirements expect a variable named ssh_key_public containing the public key content (your-public-key-content). Either change this to ssh_public_key = var.ssh_key_public (and provide the key content in variables) or ensure you have a correctly documented ssh_key_public_path variable and that it matches the checklist. This mismatch will prevent meeting the specified variable contract.
| vnet_address_prefix = var.vnet_address_prefix | ||
| subnet_address_prefix = var.subnet_address_prefix | ||
| 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 to the network module, but the checklist requires the public IP DNS label to be generated by concatenating matetask and a random number. Ensure that the randomness is implemented (for example, create a random_integer or random_id resource and build dns_label = "${var.dns_label}${random_integer.xxx.result}") either here or inside the network module. As written, var.dns_label alone may be static and not satisfy the requirement.
| resource "azurerm_network_interface_security_group_association" "nic_nsg_assoc" { | ||
| network_interface_id = module.compute.nic_id | ||
| network_security_group_id = module.network.nsg_id |
There was a problem hiding this comment.
The azurerm_network_interface_security_group_association references module.compute.nic_id and module.network.nsg_id. Confirm that modules/compute exposes nic_id and that modules/network exposes nsg_id (exact output names). If those outputs are missing or named differently the association will fail during apply.
| script_file = var.script_file | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| location = azurerm_resource_group.rg.location | ||
| subnet_id = module.network.subnet_id |
There was a problem hiding this comment.
You pass subnet_id = module.network.subnet_id and public_ip_id = module.network.public_ip_id into the compute module. Similarly, make sure modules/network defines and exports these outputs (subnet_id, public_ip_id) — otherwise the compute module will not receive the necessary IDs.
| variable "vm_name" { | ||
| description = "Name of the virtual machine." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The task requires a variable named vm_name with the value matebox. This module declares vm_name but provides no default — ensure the root variables.tf sets vm_name = "matebox" so the overall configuration meets the checklist.
| variable "vm_size" { | ||
| description = "Size/SKU of the virtual machine." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The task requires vm_size to be Standard_B1s. This variable is declared here without a default — ensure the root variables.tf or module call sets vm_size = "Standard_B1s" to comply with the requirement.
| variable "script_file" { | ||
| description = "Path to the bootstrap script used on the VM." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The VM extension must execute install-app.sh per the requirements. This file exposes a generic script_file variable but has no default. Either default this variable to install-app.sh or rename it to install-app.sh so the VM extension will call the exact filename required by the checklist.
| variable "ssh_key_public_path" { | ||
| description = "Path to the SSH public key file." | ||
| type = string | ||
| default = "../id_rsa_azure.pub" | ||
| } |
There was a problem hiding this comment.
The checklist requires a variable named ssh_key_public containing the public key content (your-public-key-content). This file defines ssh_key_public_path (a path to a file) with default ../id_rsa_azure.pub, which does not satisfy the requirement. Replace or add variable "ssh_key_public" { type = string } and provide the public key content from the root variables.tf.
| variable "ssh_key_public_path" { | ||
| description = "Path to the SSH public key file." | ||
| type = string | ||
| default = "../id_rsa_azure.pub" | ||
| } |
There was a problem hiding this comment.
If you intend to keep ssh_key_public_path here, ensure you document/implement code that reads the file and passes its contents as the ssh_key_public input to the VM resource — but the checklist specifically expects ssh_key_public to be a variable, so the safer fix is to declare that variable instead.
| variable "ssh_public_key" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The variable is named ssh_public_key here but the task's variable list (Implementation Details / Technical Constraints) requires ssh_key_public. This naming mismatch will break variable mapping or violate the checklist. Rename this variable to ssh_key_public or ensure the root variables and module usage are updated to match exactly.
| variable "script_file" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
script_file is declared but appears unused by the compute module (compute/main.tf uses a hard-coded script URL). If this variable is intended to supply the install script path/URL for the VM extension, reference var.script_file in the module; otherwise remove it to avoid unused-variable confusion.
| @@ -0,0 +1,71 @@ | |||
| resource "azurerm_virtual_network" "vnet" { | |||
| name = var.virtual_network_name | |||
There was a problem hiding this comment.
This resource uses var.virtual_network_name for the vnet name. Be sure the root variable virtual_network_name is set to vnet as required by the task so the created vnet has the exact name vnet.
| 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 subnet's address_prefixes uses var.subnet_address_prefix. Ensure subnet_address_prefix is set to 10.0.0.0/24 in your variables so the subnet matches the task constraint.
| } | ||
|
|
||
| resource "azurerm_network_security_group" "nsg" { | ||
| name = var.network_security_group_name |
There was a problem hiding this comment.
The NSG name comes from var.network_security_group_name. The task requires the NSG be named defaultnsg — ensure that variable is set accordingly in the root variables file.
| resource "random_string" "dns_suffix" { | ||
| length = 6 | ||
| upper = false | ||
| numeric = true |
There was a problem hiding this comment.
random_string is configured with numeric = true but lower defaults to true (so the result may include lowercase letters). The task requires appending a random number; constrain this suffix to digits only (e.g., set lower = false, upper = false, numeric = true, or use random_integer) so the domain label is matetask + digits.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| sku = "Standard" |
There was a problem hiding this comment.
Public IP uses allocation_method = "Dynamic" but the SKU is set to Standard. Azure Standard SKU requires static allocation; this mismatch will cause an error. To meet the task requirement of a dynamic allocation method, change sku to Basic (or set allocation to Static if you intentionally want Standard).
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| sku = "Standard" | ||
| domain_name_label = "${var.dns_label}${random_string.dns_suffix.result}" |
There was a problem hiding this comment.
The domain_name_label concatenates var.dns_label and the random suffix. Ensure var.dns_label is set to matetask in your variables so the final DNS label follows the required pattern (e.g., matetask123456).
| # Storage Account | ||
| output "storage_account_id" { | ||
| description = "ID of the Storage Account" | ||
| value = azurerm_storage_account.storage.id |
There was a problem hiding this comment.
This output references azurerm_storage_account.storage. Ensure the storage module actually defines a resource with that exact name (azurerm_storage_account "storage") — otherwise Terraform will fail with an undeclared resource error. The task requires the storage module to create a storage account, so verify it's present and named as expected.
|
|
||
| output "storage_account_primary_endpoint" { | ||
| description = "Primary blob endpoint of the Storage Account" | ||
| value = azurerm_storage_account.storage.primary_blob_endpoint |
There was a problem hiding this comment.
You expose the primary blob endpoint which is useful. Verify that the storage account is configured for blob services so this attribute exists and is meaningful for any consumers (e.g., uploads or backend checks).
| # Storage Container | ||
| output "storage_container_id" { | ||
| description = "ID of the Storage Container" | ||
| value = azurerm_storage_container.container.id |
There was a problem hiding this comment.
This output references azurerm_storage_container.container. Confirm the module defines azurerm_storage_container "container" and that the container name meets the task requirement (task-artifacts) — otherwise this output will not reflect the required container or will error if the resource is missing.
| output "storage_container_name" { | ||
| description = "Name of the Storage Container" | ||
| value = azurerm_storage_container.container.name |
There was a problem hiding this comment.
You output the container name. The task requires the storage container to be named task-artifacts. If the underlying resource uses a different name, update the resource or this output to ensure the required container is created and exposed.
| type = string | ||
| } | ||
|
|
||
| variable "container_name" { |
There was a problem hiding this comment.
The task requires the storage container to be named task-artifacts. Consider setting a default here (default = "task-artifacts") or add validation to enforce the required name so the module always creates the container with the expected name.
| @@ -0,0 +1,15 @@ | |||
| variable "storage_account_name" { | |||
There was a problem hiding this comment.
Add validation and/or a description for storage_account_name. Azure storage account names must be lowercase, 3–24 characters, and only alphanumeric. If this module is used to create the backend storage account for remote state, ensure the value matches yourstorageaccount or document that the backend storage is managed separately.
| type = string | ||
| } | ||
|
|
||
| variable "location" { |
There was a problem hiding this comment.
The task requires location to be uksouth. Consider setting default = "uksouth" here or ensure the root-level variables.tf/terraform.tfvars sets location = "uksouth" so the created storage resources meet the requirement.
| variable "resource_group_name" { | ||
| description = "Resource group name for all resources." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
These variables need the exact required default values (or be set via the root terraform.tfvars). Add default = "mate-azure-task-12" here so the resource_group_name matches the task requirement mate-azure-task-12.
| variable "location" { | ||
| description = "Azure region for all resources." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
Add the required default uksouth for location so the deployment uses the mandated region. Example: default = "uksouth".
| variable "virtual_network_name" { | ||
| description = "Name of the virtual network." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The task requires the virtual network be named vnet. Add default = "vnet" so virtual_network_name matches the checklist.
| variable "vnet_address_prefix" { | ||
| description = "Address space for the virtual network." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The task requires the vnet address prefix be 10.0.0.0/16. Add default = "10.0.0.0/16" so this variable matches the requirement.
| variable "subnet_address_prefix" { | ||
| description = "Address prefix for the subnet." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The subnet address prefix must be 10.0.0.0/24 per the checklist. Add default = "10.0.0.0/24" to ensure the subnet matches the requirement.
| variable "subnet_name" { | ||
| description = "Name of the subnet." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The subnet must be named default according to the task. Add default = "default" so subnet_name matches the checklist item.
| variable "network_security_group_name" { | ||
| description = "Name of the network security group." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The network security group name is required to be defaultnsg. Add default = "defaultnsg" so this variable uses the exact required value.
| variable "public_ip_address_name" { | ||
| description = "Name of the public IP resource." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The public IP resource must be named linuxboxpip. Add default = "linuxboxpip" here so the created public IP uses the required name.
| variable "dns_label" { | ||
| description = "DNS label prefix for the public IP." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The task requires dns_label to be matetask (you may append a random number). Add default = "matetask" so the base DNS label matches the requirement.
| @@ -0,0 +1,13 @@ | |||
| resource "azurerm_storage_account" "storage" { | |||
| name = var.storage_account_name | |||
There was a problem hiding this comment.
name = var.storage_account_name — The task requires a storage account for remote state named yourstorageaccount. Ensure var.storage_account_name is set to yourstorageaccount (and note Azure storage account names must be globally unique, 3–24 characters, and lowercase).
| @@ -0,0 +1,13 @@ | |||
| resource "azurerm_storage_account" "storage" { | |||
| name = var.storage_account_name | |||
| resource_group_name = var.resource_group_name | |||
There was a problem hiding this comment.
resource_group_name = var.resource_group_name — The remote state backend requires the resource group mate-azure-task-12. Make sure var.resource_group_name is set to mate-azure-task-12 so the storage account is created in the correct resource group.
| resource "azurerm_storage_account" "storage" { | ||
| name = var.storage_account_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location |
There was a problem hiding this comment.
location = var.location — Per the task the location must be uksouth. Confirm var.location is set to uksouth in your root variables so resources are deployed to the required region.
| } | ||
|
|
||
| resource "azurerm_storage_container" "container" { | ||
| name = var.container_name |
There was a problem hiding this comment.
name = var.container_name — The storage container for application artifacts must be named task-artifacts as per the checklist. Ensure var.container_name is set accordingly. If this module is also expected to create the Terraform backend container (tfstate), currently it does not — add a second azurerm_storage_container resource for the tfstate container or create it elsewhere.
| storage_account_name = azurerm_storage_account.storage.name | ||
| container_access_type = "private" | ||
| } |
There was a problem hiding this comment.
This module creates only one container. The backend requires a container named tfstate for remote state. Either create another azurerm_storage_container with name = "tfstate" in this module (or a separate module) or confirm it's created before terraform init uses the backend.
| @@ -0,0 +1,21 @@ | |||
| # Network configuration variables | |||
There was a problem hiding this comment.
This tfvars file is under terraform/. The Implementation Details require a root /terraform.tfvars. Move or copy this file to the repository root so Terraform and the task checker pick it up.
|
|
||
| # Storage configuration variables | ||
| storage_account_name = "yourstorageaccount" | ||
| container_name = "task-artifacts" |
There was a problem hiding this comment.
The backend remote-state container must be named tfstate per the task. Here container_name is set to task-artifacts, which appears to be for application artifacts. Change or add a variable so the remote state uses tfstate (and ensure that container exists in the storage account).
| vm_size = "Standard_B1s" | ||
| admin_username = "whatislavx" | ||
| script_file = "install-app.sh" | ||
| ssh_key_public_path = "../id_rsa_azure.pub" |
There was a problem hiding this comment.
You provide the SSH public key as a file path variable ssh_key_public_path. The task and other modules expect a variable named ssh_key_public containing the public-key content (or at minimum consistent naming). Rename/replace this with ssh_key_public and provide the key content, or update modules to consume the path and load the file consistently during provisioning.
| vm_name = "matebox" | ||
| vm_size = "Standard_B1s" | ||
| admin_username = "whatislavx" | ||
| script_file = "install-app.sh" |
There was a problem hiding this comment.
You declare script_file = "install-app.sh". Ensure the compute module's VM extension uses this variable (or the exact filename install-app.sh) so the CustomScript extension executes the expected script as required by the checklist.
| variable "container_name" { | ||
| description = "Name of the blob container." | ||
| type = string |
There was a problem hiding this comment.
There are no default values set. The task requires the storage container used by the storage module to be named task-artifacts. Add default = "task-artifacts" to container_name (or ensure the root terraform.tfvars sets it).
| variable "storage_account_name" { | ||
| description = "Name of the storage account." | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The storage_account_name variable must correspond to the storage account used to host the artifact container. If this is meant to be the artifact storage account set its default to the required value (or document where the remote-state storage account yourstorageaccount is declared). Consider adding default = "yourstorageaccount" if this variable is intended for backend remote state, otherwise add a separate variable for the remote backend storage account.
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is empty. Add outputs exposing the VM public IP address and DNS/FQDN (so the app can be accessed after deployment). Also consider outputs for vm_name, storage_account_name, and storage_container_name to aid verification. Example output to add: ```hcl
output "vm_public_ip" {
description = "Public IP address of the VM"
value = module.network.public_ip_address // or reference your resource/module
}
output "vm_public_fqdn" {
description = "Public DNS name of the VM"
value = module.network.public_ip_fqdn
}
``` Adjust the value expressions to match your module/output names.
https://github.com/whatislavx/devops_todolist_terraform_task/actions/runs/21915692845/job/63282188343