Solution#62
Conversation
formating files
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am asking for changes because there are several critical issues that will block a successful deployment and fail the task’s explicit requirements. Most importantly, the backend configuration (in backend.tf and related references) uses storage_account_name = "mykytastorage" instead of the required yourstorageaccount with container tfstate, and that storage account/container must exist before terraform init. Across variables.tf, terraform.tfvars, main.tf, and the module files you consistently use misspelled variable names like vnet_adress_prefix, subnet_adress_prefix, and public_ip_adress_name instead of the required vnet_address_prefix, subnet_address_prefix, and public_ip_address_name, which breaks variable mapping and violates the checklist. The VM extension and install-app.sh together are functionally broken: commandToExecute concatenates commands incorrectly (so systemctl restart todoapp is passed as an argument instead of a separate command), the script uses invalid flags for apt-get update, never installs git before cloning, and the git clone line targets the wrong repo/path and is redundant with the extension’s own clone, so the app will not be deployed correctly. In the storage-related code (e.g., modules/storage/main.tf, modules/network/variables.tf, and outputs), storage account names like matetask-${random_id.storage_suffix.hex} include a hyphen, which is invalid for Azure storage accounts, and one of the storage container resource labels uses a hyphen (task-artifacts), which is not a valid Terraform identifier; these will cause terraform apply to fail. The public IP resource is also configured with allocation_method = "Static" instead of the required "Dynamic", and the SSH key handling does not match the task’s expectation (key name linuxboxsshkey and placeholder your-public-key-content).
To move this forward, you should: (1) fix the backend block to use the exact required storage account/container and ensure they exist; (2) rename all adress variables to the exact address spellings everywhere (definitions, uses, and terraform.tfvars); (3) correct the VM extension command to chain with && and repair install-app.sh (valid apt flags, install git, remove or fix the clone and use the expected /devops_todolist_terraform_task/app/* path); (4) update storage account names and related outputs to be hyphen-free, lowercase alphanumeric, and within length limits; and (5) set the public IP allocation to Dynamic and align SSH key naming/defaults with the task. Once these changes are in place, your overall module structure should satisfy the requirements and you’ll be in a good position to pass the checklist and verify the app over the VM’s public IP.
✨ 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
| 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/betterthink/azure_task_12_deploy_app_with_vm_extention.git |
There was a problem hiding this comment.
This git clone is incorrect and incomplete. The compute module's extension already clones the correct repository into /devops_todolist_terraform_task, so this line is redundant and will either clone the wrong repo or create an unexpected directory. Remove this line or update it to: git clone https://github.com/betterthink/devops_todolist_terraform_task /devops_todolist_terraform_task so it matches the later cp usage.
| "zh:481376d79224a0e4aebc6e39dee10de3cc14efd1c7c58b6d74c538e356cf4bb2", | ||
| "zh:625119aec0d24ff693c589d802b7983ffce3fcf1e9c3351396af02799dd176ca", | ||
| "zh:65981e62a6e9eb8a39dd332632617e8c929dcce6af48d3829f590f5c0f14362f", | ||
| "zh:72db82697f4e694c21defa8d0efb22f71d373676d078d71d567e8b4d9a134df7", |
There was a problem hiding this comment.
The git clone command is wrong/mismatched and missing a destination. You clone https://github.com/betterthink/azure_task_12_deploy_app_with_vm_extention.git but then copy from devops_todolist_terraform_task/app/* — that directory won't exist. Also the VM extension already clones the repo before running this script, so either remove this clone line or change it to clone the same devops_todolist_terraform_task repo into that directory (e.g. git clone https://github.com/betterthink/devops_todolist_terraform_task devops_todolist_terraform_task).
|
|
||
| .terraform/ | ||
| *.tfstate | ||
| *.tfstate.* |
There was a problem hiding this comment.
The backend configuration sets storage_account_name = "mykytastorage" but the task requires the backend to use the storage account name yourstorageaccount (and resource group mate-azure-task-12, container tfstate, key terraform.tfstate). Update this value to the required name and ensure that the storage account and container exist (the backend requires the storage container to be present before init).
| ] | ||
|
|
||
| admin_ssh_key { | ||
| username = "adminuser" |
There was a problem hiding this comment.
This uses vnet_adress_prefix (misspelled). The task requires the variable name vnet_address_prefix (with double 'd'). Rename this variable here and in variables.tf and the network module to match the requirement.
| public_key = var.ssh_key_public | ||
| } | ||
|
|
||
| os_disk { |
There was a problem hiding this comment.
This uses subnet_adress_prefix (misspelled). The task requires subnet_address_prefix. Rename this variable here and ensure the network module and variables.tf use the exact required name.
| } | ||
|
|
||
| os_disk { | ||
| caching = "ReadWrite" |
There was a problem hiding this comment.
This uses public_ip_adress_name (misspelled). The task requires public_ip_address_name. Update this field here and in variables.tf and the network module so the variable name matches the task specification exactly.
| source = "./modules/network" | ||
|
|
||
| virtual_network_name = var.virtual_network_name | ||
| vnet_adress_prefix = var.vnet_adress_prefix |
There was a problem hiding this comment.
This argument uses vnet_adress_prefix (misspelled) — the task explicitly requires the variable name vnet_address_prefix (with "address"). Use the exact variable name required by the task so your configuration matches the checklist.
| location = azurerm_resource_group.rg.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| subnet_name = var.subnet_name | ||
| subnet_adress_prefix = var.subnet_adress_prefix |
There was a problem hiding this comment.
This argument uses subnet_adress_prefix (misspelled). The task requires subnet_address_prefix. Rename to the exact variable name expected by the assignment so the variables file and module usage match the checklist.
| resource_group_name = azurerm_resource_group.rg.name | ||
| subnet_name = var.subnet_name | ||
| subnet_adress_prefix = var.subnet_adress_prefix | ||
| public_ip_adress_name = var.public_ip_adress_name |
There was a problem hiding this comment.
This argument uses public_ip_adress_name (misspelled). The task requires public_ip_address_name. Update to the exact name required by the task so the variable definitions and module inputs align.
| source = "./modules/storage" | ||
| location = azurerm_resource_group.rg.location | ||
| resource_group_name = azurerm_resource_group.rg.name | ||
| } No newline at end of file |
There was a problem hiding this comment.
The VM extension's commandToExecute concatenates commands incorrectly: bash /devops_todolist_terraform_task/install-app.sh systemctl restart todoapp will pass systemctl restart todoapp as an argument to the script rather than run it after the script completes. Chain commands explicitly (for example: ... && bash /devops_todolist_terraform_task/install-app.sh && systemctl restart todoapp) so the restart runs after the install script succeeds.
| terraform { | ||
| required_providers { | ||
| azurerm = { | ||
| source = "hashicorp/azurerm" |
There was a problem hiding this comment.
The backend configuration (in backend.tf) sets storage_account_name = "mykytastorage", but the task requires the backend storage account to be yourstorageaccount (and the resource group mate-azure-task-12, container tfstate, key terraform.tfstate). Update the backend to match the required values and ensure the storage account + container exist before running terraform init.
| } | ||
| variable "vnet_adress_prefix" { | ||
| type = string | ||
| } |
There was a problem hiding this comment.
The storage account name is defined as "matetask-${random_id.storage_suffix.hex}" which inserts a hyphen. Azure storage account names must be 3-24 characters, only lowercase letters and numbers (no hyphens). Change to a hyphen-free, lowercase alphanumeric name, for example: name = "matetask${random_id.storage_suffix.hex}" and ensure it meets the length constraints.
| variable "resource_group_name" { | ||
| type = string | ||
| } | ||
| variable "subnet_name" { |
There was a problem hiding this comment.
The Terraform resource label azurerm_storage_container "task-artifacts" uses a hyphen. Terraform resource labels should avoid hyphens (use underscores) to prevent parsing/identifier issues. Rename the resource label to something like task_artifacts while keeping the container name = "task-artifacts" (the container's name is allowed to contain a hyphen).
| } | ||
|
|
||
| resource "azurerm_storage_account" "st_acc" { | ||
| name = "matetask-${random_id.storage_suffix.hex}" |
There was a problem hiding this comment.
The storage account name includes a hyphen (matetask-${random_id.storage_suffix.hex}) which is invalid for Azure storage account names (they must be 3-24 chars and contain only lowercase letters and numbers). Remove the hyphen and ensure the generated name conforms to Azure rules (e.g., matetask${random_id.storage_suffix.hex}) so the resource will be created successfully.
| resource "random_id" "server_suffix" { | ||
| byte_length = 4 | ||
| } | ||
|
|
There was a problem hiding this comment.
The backend storage account name here is mykytastorage. The task requires the backend to use storage account yourstorageaccount (and container tfstate) and that this storage container exist before terraform init. Update the backend to the required storage account name or ensure the referenced storage account/container are created prior to init.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| security_rule { | ||
| name = "allow-ssh" |
There was a problem hiding this comment.
In the root main.tf you pass vnet_adress_prefix to the network module. This variable name is misspelled; the task requires vnet_address_prefix (with "address"). Use the exact variable name required by the specification so values are consistent across files and with the checklist.
| priority = 100 | ||
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" |
There was a problem hiding this comment.
In the root main.tf you pass subnet_adress_prefix (misspelled). The task requires subnet_address_prefix. Rename to the exact variable name expected by the assignment so your configuration matches the checklist.
| direction = "Inbound" | ||
| access = "Allow" | ||
| protocol = "Tcp" | ||
| source_port_range = "*" |
There was a problem hiding this comment.
In the root main.tf you pass public_ip_adress_name (misspelled). The task requires public_ip_address_name. Update this input name here and ensure the variable declaration names match exactly.
|
|
||
| resource "azurerm_virtual_network" "vnet" { | ||
| name = var.virtual_network_name | ||
| address_space = [var.vnet_adress_prefix] |
There was a problem hiding this comment.
The network module uses var.vnet_adress_prefix (misspelled). Rename to vnet_address_prefix to match the task requirement and the root variable names.
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = [var.subnet_adress_prefix] |
There was a problem hiding this comment.
The network module uses var.subnet_adress_prefix (misspelled). The required name is subnet_address_prefix; align the variable name and usages accordingly.
| network_security_group_id = azurerm_network_security_group.defaultnsg.id | ||
| } | ||
| resource "azurerm_public_ip" "example" { | ||
| name = var.public_ip_adress_name |
There was a problem hiding this comment.
The network module uses var.public_ip_adress_name (misspelled). The required variable name is public_ip_address_name; update the variable and usages to match the task specification.
| name = var.public_ip_adress_name | ||
| resource_group_name = var.resource_group_name | ||
| location = var.location | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
The public IP allocation is set to Static but the task requires dynamic allocation. Change allocation_method = "Static" to allocation_method = "Dynamic" for the public IP resource.
| network_security_group_id = azurerm_network_security_group.defaultnsg.id | ||
| } | ||
| resource "azurerm_public_ip" "example" { | ||
| name = var.public_ip_adress_name |
There was a problem hiding this comment.
The VM extension commandToExecute concatenates commands incorrectly: bash /devops_todolist_terraform_task/install-app.sh systemctl restart todoapp will pass systemctl restart todoapp as an argument to the script rather than running it afterwards. Chain commands explicitly (for example && bash .../install-app.sh && systemctl restart todoapp) so the restart runs after the install succeeds.
| address_space = [var.vnet_adress_prefix] | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| } |
There was a problem hiding this comment.
The install script runs apt-get update -yq. apt-get update does not accept the -y flag; use apt-get update -q or simply apt-get update, and use apt-get -yq upgrade if you intend to upgrade packages non-interactively. Correct the flags to avoid errors or unexpected behavior.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| } | ||
|
|
There was a problem hiding this comment.
The script does not install git before attempting to clone. Add apt-get install git -yq (or include git in the existing install command) so the git clone will succeed on a fresh VM.
| name = var.subnet_name | ||
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = [var.subnet_adress_prefix] |
There was a problem hiding this comment.
The git clone line is incorrect/mismatched and missing a destination. It clones azure_task_12_deploy_app_with_vm_extention.git but later the script copies from devops_todolist_terraform_task/app/*. Ensure the clone target and the cp source match. Either remove this clone (the VM extension already clones the repo) or clone the correct repo into devops_todolist_terraform_task (for example: git clone https://github.com/betterthink/devops_todolist_terraform_task /devops_todolist_terraform_task).
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name | ||
| address_prefixes = [var.subnet_adress_prefix] | ||
| } |
There was a problem hiding this comment.
This cp copies from devops_todolist_terraform_task/app/* but given the current git clone above that directory may not exist. Make sure the cloned repo path matches this cp source or adjust the cp path to the actual cloned folder. Also consider using mkdir -p /app and checking that the source exists before copying to avoid silent failures.
| resource_group_name = var.resource_group_name | ||
| virtual_network_name = azurerm_virtual_network.vnet.name |
There was a problem hiding this comment.
The storage module creates a storage account and a container named task-artifacts, but the Terraform backend expects a storage account and a container named tfstate for remote state. The backend container must exist before terraform init. Either create the backend storage account and a tfstate container (with the required name yourstorageaccount/tfstate) or change the backend to point to an account/container that will exist.
| } | ||
| output "nsg_id" { | ||
| value = azurerm_network_security_group.defaultnsg.id | ||
| } |
There was a problem hiding this comment.
Azure storage account names cannot contain hyphens and must be lowercase letters and numbers only. "matetask-${random_id.storage_suffix.hex}" includes a hyphen which will make the resource invalid. Remove the hyphen (for example: "matetask${random_id.storage_suffix.hex}") or choose a name that only uses lowercase letters and digits and stays within the 3–24 char limit.
| variable "location" { | ||
| type = string | ||
| } | ||
| variable "resource_group_name" { |
There was a problem hiding this comment.
Variable name is misspelled: vnet_adress_prefix should be vnet_address_prefix to match the task requirements and the variable declared in variables.tf. Use the exact name vnet_address_prefix so Terraform maps this value correctly.
| } | ||
| variable "resource_group_name" { | ||
| type = string | ||
| } No newline at end of file |
There was a problem hiding this comment.
Variable name is misspelled: subnet_adress_prefix should be subnet_address_prefix as required by the task. Rename this to subnet_address_prefix so it matches the variable declaration and module inputs.
| output "storage_account_name" { | ||
| value = azurerm_storage_account.st_acc.name | ||
| } | ||
|
|
There was a problem hiding this comment.
The variable name is misspelled: vnet_adress_prefix should be vnet_address_prefix (task requires that exact name). Rename this key so Terraform and the task checks receive the expected variable.
| } | ||
|
|
||
| output "storage_container_name" { | ||
| value = azurerm_storage_container.task-artifacts.name |
There was a problem hiding this comment.
The variable name is misspelled: subnet_adress_prefix should be subnet_address_prefix (task requires that exact name). Update this key to match the required variable name.
| output "resource_group_name" { | ||
| value = azurerm_resource_group.rg.name | ||
| } | ||
| output "storage_account_name" { |
There was a problem hiding this comment.
Variable name is misspelled: vnet_adress_prefix should be vnet_address_prefix to match the task specification. Use the exact variable name required by the assignment so automated checks and other modules receive the value.
| } | ||
| output "storage_account_name" { | ||
| value = module.storage.storage_account_name | ||
| } |
There was a problem hiding this comment.
Variable name is misspelled: subnet_adress_prefix should be subnet_address_prefix per the task requirements. Rename this key to match the exact name expected by the assignment.
| value = module.storage.storage_account_name | ||
| } | ||
| output "public_ip_fqdn" { | ||
| value = module.network.publicip_fqdn |
There was a problem hiding this comment.
Variable name is misspelled: public_ip_adress_name should be public_ip_address_name according to the requirements. Update this key to the exact name required so the network module receives the correct input.
| location = "uksouth" | ||
| resource_group_name = "mate-azure-task-12" | ||
| virtual_network_name = "vnet" | ||
| vnet_adress_prefix = "10.0.0.0/16" |
There was a problem hiding this comment.
Variable name is misspelled. The task requires vnet_address_prefix (with "address"). Rename vnet_adress_prefix to vnet_address_prefix so it matches the required variable name.
| virtual_network_name = "vnet" | ||
| vnet_adress_prefix = "10.0.0.0/16" | ||
| subnet_name = "default" | ||
| subnet_adress_prefix = "10.0.0.0/24" |
There was a problem hiding this comment.
Variable name is misspelled. The task requires subnet_address_prefix (with "address"). Rename subnet_adress_prefix to subnet_address_prefix to match the specification.
| subnet_name = "default" | ||
| subnet_adress_prefix = "10.0.0.0/24" | ||
| network_security_group_name = "defaultnsg" | ||
| public_ip_adress_name = "linuxboxpip" |
There was a problem hiding this comment.
Variable name is misspelled. The task requires public_ip_address_name (with "address"). Rename public_ip_adress_name to public_ip_address_name so the modules receive the correct input.
| default = "vnet" | ||
| } | ||
|
|
||
| variable "vnet_adress_prefix" { |
There was a problem hiding this comment.
Variable name is misspelled. Rename vnet_adress_prefix to the exact required name vnet_address_prefix so Terraform and automated checks receive the expected variable.
| type = string | ||
| default = "default" | ||
| } | ||
| variable "subnet_adress_prefix" { |
There was a problem hiding this comment.
Variable name is misspelled. Rename subnet_adress_prefix to the exact required name subnet_address_prefix to match the task specification.
| default = "defaultnsg" | ||
| } | ||
|
|
||
| variable "public_ip_adress_name" { |
There was a problem hiding this comment.
Variable name is misspelled. Rename public_ip_adress_name to the exact required name public_ip_address_name so the public IP name is passed correctly to the network module.
| variable "ssh_key_public" { | ||
| type = string | ||
| sensitive = true | ||
| default = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDdZGrZdK1h2zFrTUedM3KgFTUayGtbANXpZ0Tqe3IDgqwB6sffFpmNN5LItWDADkSfoc0xGvBPrgT9pSqbnnK4zayNN/esg2kY9EIwPoBzbhYWAvs+4hDgQRtx3gpppxWZZYQmKg7jXpLCC/BegZnZCRO2qDEuW+QkUUdADYVm8QB+VX+ow7rqp+CjqJcwaX+6hKUXK58Sim7FZtqzeI1QtpVOTUpFYArJkrMkUKeqi3+WSq42s3DILBtfRsveIF8NNNZmb41nkEuZNYIzLHa/jD56CkqmNjR+r2RBHxoMfJQcLYmNlXAXY2OSjYUFRb1lIdAyPr0leimSjfZ80Ge5qBfT0uBKnQ6FqcZS1avDC8WNKaMu4dyIMJ0bGm0uqQ0poioV0UfhI9taPZ7dbY45kG8zaQ9iqpyGYrUWFn89mChDALaX/p41qLH4c0KlUBGIIu4FAH4k+Zo+Xy6DvEKz7omMC7T/KW51k1gmy7k5hQNVGGW+FVlqDEDLLfcG5M1SoEw+iT09NgWUwo/TsCvshWpwpd5aUoktlAKpO1bFIYXgnd4xaUwN4pqeFp5fq5XKDqYYu3da90EVOSMeytmRBwnLLZI2JzCU8fArOqNxEcNErHK8W4e2vHwZT3divEdz9/tpAODVXFePpe77lNZx5+44L4OgKYjcwvRxo0R7Sw== remarkt@GEBRUIK-5LSA2K3" |
There was a problem hiding this comment.
The ssh_key_public variable has a long SSH key as its default. The task expects ssh_key_public to be your-public-key-content (or supplied via terraform.tfvars). Consider setting the default to the placeholder "your-public-key-content" or removing the default so users must provide the key in terraform.tfvars.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical issues that will prevent the infrastructure and app from working as required. In install-app.sh, apt-get update is called with invalid flags, git is never installed before it is used, the repository you clone does not match the later cp path, and the script uses non‑idempotent operations (like mkdir /app and mv without checks) — together these will cause the Custom Script extension to fail rather than reliably deploying the ToDo app. In modules/compute/main.tf, the VM extension’s commandToExecute runs git clone before git is installed and then restarts the todoapp service outside of the install script, so the clone can fail and the service can be restarted before the unit file and app files exist; you should either move cloning and all systemctl actions into install-app.sh (after installing git) or ensure the extension only invokes a robust install script. In the network module, the public IP is configured with allocation_method = "Static", but the task requires a Dynamic public IP, so this must be changed to "Dynamic" to meet the checklist. SSH key handling also violates the requirements: variables.tf does not set the default ssh_key_public = "your-public-key-content", terraform.tfvars contains a real SSH public key instead of the placeholder, and there is no azurerm_ssh_public_key resource named linuxboxsshkey, so you need to add the placeholder default, remove/replace the committed key, and create/use the required key resource. Additionally, backend.tf points at the fixed backend storage account yourstorageaccount and container tfstate, but Terraform requires these to exist before terraform init, so you must ensure they are created out-of-band or adjust the backend to a storage account/container that will exist. On top of these blockers, please harden install-app.sh with set -euo pipefail at the top, use mkdir -p /app, and add simple existence checks/fail-fast messages around the expected /devops_todolist_terraform_task/app/* path so errors are clearer. Once you address these points, your overall module structure (network/compute/storage) is on the right track and should satisfy the task requirements more reliably.
✨ 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
| @@ -13,7 +13,7 @@ apt-get install python3-pip -yq | |||
| # Create a directory for the app and download the files. | |||
| mkdir /app | |||
There was a problem hiding this comment.
Use mkdir -p /app so the command is idempotent and won't fail if /app already exists.
| 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/betterthink/azure_task_12_deploy_app_with_vm_extention.git |
There was a problem hiding this comment.
The git clone target repo and resulting directory name don't match the cp path used later. Also this clone may be redundant because the VM extension already clones the repo. Either remove this clone from the script or clone the repo into /devops_todolist_terraform_task (matching the cp line), and make the repo URL configurable (use a variable passed from Terraform) so it matches github_repo.
| # 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/betterthink/azure_task_12_deploy_app_with_vm_extention.git | ||
| cp -r devops_todolist_terraform_task/app/* /app |
There was a problem hiding this comment.
The cp -r devops_todolist_terraform_task/app/* /app will fail if the expected devops_todolist_terraform_task folder doesn't exist. Ensure the clone creates that folder or update the path to match the actual cloned directory. Add checks or fail-fast messages to make errors clearer.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "yourstorageaccount" |
There was a problem hiding this comment.
The backend is using the required storage_account_name value yourstorageaccount, which is correct per the task, but Terraform requires that the storage account and the tfstate container already exist before terraform init. Ensure you create that storage account and container ahead of time (or create them outside of this backend block) to avoid init failing.
| @@ -0,0 +1,8 @@ | |||
| terraform { | |||
There was a problem hiding this comment.
Add strict/fail-fast behavior to the script so errors stop execution early. Consider adding at the top: set -euo pipefail (or at minimum set -e). This prevents the script from continuing after a failed command.
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "commandToExecute": " cd / && git clone ${var.github_repo} devops_todolist_terraform_task && chmod +x /devops_todolist_terraform_task/install-app.sh && bash /devops_todolist_terraform_task/install-app.sh && systemctl restart todoapp" |
There was a problem hiding this comment.
The VM extension's command performs git clone before install-app.sh runs. Since git is not installed prior to this clone (and your install script currently installs packages), the clone will fail. Either move the git clone into install-app.sh after installing git, or ensure git is installed before running git clone in this command. Also ensure the cloned directory matches the cp path used by the script (use /devops_todolist_terraform_task).
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "commandToExecute": " cd / && git clone ${var.github_repo} devops_todolist_terraform_task && chmod +x /devops_todolist_terraform_task/install-app.sh && bash /devops_todolist_terraform_task/install-app.sh && systemctl restart todoapp" |
There was a problem hiding this comment.
The extension command ends with && systemctl restart todoapp. Restarting the service from the extension risks running before the unit file and app files are in place. It's more reliable to have install-app.sh perform systemctl daemon-reload and systemctl enable --now todoapp after placing the service file and app files. Remove or move service management into the install script so it runs where the files are guaranteed to exist.
|
|
||
| variable "github_repo" { | ||
| type = string |
There was a problem hiding this comment.
The task checklist requires the VM to use an SSH key named linuxboxsshkey. Here you provide the public key via var.ssh_key_public (admin_ssh_key block) but there is no azurerm_ssh_public_key resource named linuxboxsshkey. If the checklist expects an azurerm_ssh_public_key resource, create that resource with the name linuxboxsshkey and reference it or otherwise document how this requirement is satisfied.
| public_ip_address_name = "linuxboxpip" | ||
| vm_name = "matebox" | ||
| vm_size = "Standard_B1s" | ||
| ssh_key_public = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDdZGrZdK1h2zFrTUedM3KgFTUayGtbANXpZ0Tqe3IDgqwB6sffFpmNN5LItWDADkSfoc0xGvBPrgT9pSqbnnK4zayNN/esg2kY9EIwPoBzbhYWAvs+4hDgQRtx3gpppxWZZYQmKg7jXpLCC/BegZnZCRO2qDEuW+QkUUdADYVm8QB+VX+ow7rqp+CjqJcwaX+6hKUXK58Sim7FZtqzeI1QtpVOTUpFYArJkrMkUKeqi3+WSq42s3DILBtfRsveIF8NNNZmb41nkEuZNYIzLHa/jD56CkqmNjR+r2RBHxoMfJQcLYmNlXAXY2OSjYUFRb1lIdAyPr0leimSjfZ80Ge5qBfT0uBKnQ6FqcZS1avDC8WNKaMu4dyIMJ0bGm0uqQ0poioV0UfhI9taPZ7dbY45kG8zaQ9iqpyGYrUWFn89mChDALaX/p41qLH4c0KlUBGIIu4FAH4k+Zo+Xy6DvEKz7omMC7T/KW51k1gmy7k5hQNVGGW+FVlqDEDLLfcG5M1SoEw+iT09NgWUwo/TsCvshWpwpd5aUoktlAKpO1bFIYXgnd4xaUwN4pqeFp5fq5XKDqYYu3da90EVOSMeytmRBwnLLZI2JzCU8fArOqNxEcNErHK8W4e2vHwZT3divEdz9/tpAODVXFePpe77lNZx5+44L4OgKYjcwvRxo0R7Sw== remarkt@GEBRUIK-5LSA2K3" |
There was a problem hiding this comment.
This line contains a full SSH public key committed into the repository. The task requires the placeholder your-public-key-content for ssh_key_public, and in any case you should avoid committing real keys into repo. Replace this value with the placeholder or provide the real key securely at apply time (and add any secret-containing tfvars to .gitignore or use a secure variable mechanism).
| variable "ssh_key_public" { | ||
| type = string | ||
| sensitive = true | ||
|
|
||
| } |
There was a problem hiding this comment.
The task requires ssh_key_public to be defined with the placeholder default value your-public-key-content. Add default = "your-public-key-content" to this variable block (and avoid committing actual SSH keys into source). Keeping the variable marked sensitive = true is appropriate.
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, but there are a few important issues to address before this would be production‑ready. You did well structuring the Terraform into network, compute, and storage modules, wiring variables and outputs correctly, and hardening the script with set -euo pipefail and mkdir -p /app. A critical issue is in install-app.sh (around the first apt-get call, e.g. lines 5–7) where you use apt-get update -yq; update does not accept -y, so you should run apt-get update (optionally -qq) and then a separate apt-get install -yq git python3-pip .... Another key problem is that git is not installed in install-app.sh (middle of the file, near the package install section) even though the deployment assumes a cloned repo at /devops_todolist_terraform_task/app, and the script copies from that path without first checking that the directory exists and is non‑empty—add explicit if [ -d ... ] checks and clear error messages before cp. Similarly, at the bottom of install-app.sh where you mv the todoapp.service file and immediately call systemctl daemon-reload / start / enable, this is non‑idempotent and fragile; prefer cp or install with existence checks, then only run the systemctl commands if the unit file is present and daemon-reload succeeds. In backend.tf, the remote backend points to a storage account/container (yourstorageaccount / tfstate) that this configuration does not create, so terraform init will fail unless you either pre-create that account/container or temporarily switch to a local backend. Overall, your resource definitions and module wiring look strong; once you fix the script robustness and backend mismatch, this will be a solid, repeatable deployment, and you’re clearly on the right track—keep going.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| "zh:2f81bca6a3bf3d37604bf99fdb2c77d6118520aa379ab65fd28e6b76bed399cd", | ||
| "zh:3578eb79d175af9544b0dc543124d551c0fed4c48f51773ee17e1dc62e22833a", | ||
| "zh:377dbb56caea3fa1e6a6599193b55c8594204c40c054fc2ace4f576fdfe750a2", | ||
| "zh:3d1cf01929cb213ff9a0f9753e35699bf13f60f7f0f15b38f1b216fa2cbb5f72", |
There was a problem hiding this comment.
apt-get update should not be run with -yq. apt-get update does not accept -y; run apt-get update (optionally with -qq) and then apt-get install -yq ... for package installs.
| "zh:3578eb79d175af9544b0dc543124d551c0fed4c48f51773ee17e1dc62e22833a", | ||
| "zh:377dbb56caea3fa1e6a6599193b55c8594204c40c054fc2ace4f576fdfe750a2", | ||
| "zh:3d1cf01929cb213ff9a0f9753e35699bf13f60f7f0f15b38f1b216fa2cbb5f72", | ||
| "zh:481376d79224a0e4aebc6e39dee10de3cc14efd1c7c58b6d74c538e356cf4bb2", |
There was a problem hiding this comment.
Install git (and other packages) before any clone/copy operations. For example: apt-get install -yq git python3-pip. Without git installed, any cloning performed by the extension or script will fail.
| "zh:65981e62a6e9eb8a39dd332632617e8c929dcce6af48d3829f590f5c0f14362f", | ||
| "zh:72db82697f4e694c21defa8d0efb22f71d373676d078d71d567e8b4d9a134df7", | ||
| "zh:a0fa43cf78716ff98eccd7506b017c5c487034d9d9cce88c1accdba9314a4822", | ||
| "zh:b073f60b68b0102128815251dd895ec7f24bddec84a1b725fc0777de8a78dc7e", |
There was a problem hiding this comment.
The git clone line is commented out here, but the script still assumes the repo exists at /devops_todolist_terraform_task. Either perform the git clone inside this script (after installing git) or add an explicit existence check and a clear error message if the repo path is missing.
| "zh:72db82697f4e694c21defa8d0efb22f71d373676d078d71d567e8b4d9a134df7", | ||
| "zh:a0fa43cf78716ff98eccd7506b017c5c487034d9d9cce88c1accdba9314a4822", | ||
| "zh:b073f60b68b0102128815251dd895ec7f24bddec84a1b725fc0777de8a78dc7e", | ||
| "zh:b601e509eb9735756b6b7ccacc15d6333769a7bb2f8ac8c394e39b29cfc6ee55", |
There was a problem hiding this comment.
Copying from /devops_todolist_terraform_task/app/* will fail if that path doesn't exist. Add a check such as if [ ! -d /devops_todolist_terraform_task/app ]; then echo "repo/app not found" >&2; exit 1; fi so failures are clear and fail fast.
|
|
||
| provider "registry.terraform.io/hashicorp/random" { | ||
| version = "3.8.1" | ||
| constraints = "3.8.1" |
There was a problem hiding this comment.
Service management assumes the unit file is present in /etc/systemd/system/. Rather than mv (non‑idempotent), use cp -f /app/todoapp.service /etc/systemd/system/ and check the file exists before running systemctl daemon-reload / start / enable / restart to avoid race conditions and make the script idempotent.
| # when running the script as VM extention. | ||
| set -euo pipefail | ||
|
|
||
| apt-get update -yq |
There was a problem hiding this comment.
apt-get update is invoked with -yq which is invalid: apt-get update does not accept -y. Split update and install so update runs correctly. Example: apt-get update -qq or apt-get update && apt-get install -yq ....
| set -euo pipefail | ||
|
|
||
| apt-get update -yq | ||
| apt-get install python3-pip -yq |
There was a problem hiding this comment.
Only python3-pip is installed here but the deployment expects git to be available for cloning. Install git as well (e.g. apt-get install -yq git python3-pip) and ensure this runs after a successful apt-get update.
| # git clone https://github.com/<your-gh-username>/azure_task_12_deploy_app_with_vm_extention.git | ||
| cp -r devops_todolist_terraform_task/app/* /app | ||
| #git clone https://github.com/betterthink/azure_task_12_deploy_app_with_vm_extention.git | ||
| cp -r /devops_todolist_terraform_task/app/* /app |
There was a problem hiding this comment.
This cp assumes /devops_todolist_terraform_task/app exists and contains files. Add a check for existence and non-emptiness before copying so the script fails with a clear message instead of a cryptic error. Example: if [ -d "/devops_todolist_terraform_task/app" ] && [ "$(ls -A /devops_todolist_terraform_task/app)" ]; then cp -r ...; else echo "source app dir missing" >&2; exit 1; fi.
| cp -r /devops_todolist_terraform_task/app/* /app | ||
|
|
||
| # create a service for the app via systemctl and start the app | ||
| mv /app/todoapp.service /etc/systemd/system/ |
There was a problem hiding this comment.
Using mv to install the systemd unit is non-idempotent (and will fail if the source is missing or already moved). Prefer cp or install -m 644 /app/todoapp.service /etc/systemd/system/ and check the file exists before copying to make the script idempotent and safe to re-run.
| systemctl start todoapp | ||
| systemctl enable todoapp | ||
| systemctl restart todoapp |
There was a problem hiding this comment.
systemctl start/enable/restart should only run after the unit file and application files are present and after systemctl daemon-reload completed successfully. Add checks to verify the unit file exists (and systemctl status / conditionals) before starting/restarting to avoid failing the extension run.
2 participants
https://github.com/betterthink/devops_todolist_terraform_task/actions/runs/24613525162