feat: solution for task#61
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this can be considered complete because several configuration and scripting problems will either break the deployment or violate explicit task requirements.
- In
backend.tf, thestorage_account_nameis set to"prydolobba"instead of the required"yourstorageaccount", and you must ensure thetfstatecontainer exists there soterraform initcan use the remote backend as specified. - The VM size is wrong in multiple places:
variables.tf(default),terraform.tfvars, andoutputs.tfall useStandard_B2ts_v2instead of the requiredStandard_B1s, so you need to align all of these toStandard_B1sto match the task. - In the network module (
modules/network/main.tf), the public IP resource currently hasallocation_method = "Static"but the requirement isDynamic, so update that attribute accordingly. - The
install-app.shlogic (both in the root script and the version embedded in the CustomScript command) is currently unreliable:apt-get update -yquses invalid flags,gitis not installed beforegit clone, the clone points to a different repo and no explicit target directory, and thecpsource path to the app (devops_todolist_terraform_task/app/*) does not match where the extension actually clones (/devops_todolist_terraform_task)—please fix this by (1) either removing the clone from the script and relying on the extension’s clone, or installing git and using the same repo URL/target, and (2) copying from a consistent, absolute path (e.g./devops_todolist_terraform_task/app/*) withmkdir -p /appand checks thattodoapp.serviceexists before moving it. - In
variables.tf, thessh_key_publicvariable is declared but missing the required default of"your-public-key-content", so add that placeholder (keeping itsensitive = true) or clearly ensure it is always provided viaterraform.tfvarsin line with the task instructions.
Once you address these blockers, your module structure (network/compute/storage wiring, Ubuntu 22.04 image, DNS label logic, etc.) should be in good shape, and the deployment will be much more reliable—your current layout shows solid progress, you just need to tighten these configuration and script details to fully meet the specification.
✨ 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
| override.tf | ||
| override.tf.json | ||
| *_override.tf | ||
| *_override.tf.json |
There was a problem hiding this comment.
If you keep the git clone inside this script, you must ensure git is installed. Either remove the clone (recommended, since the extension already clones), or install git here: apt-get install git -yq before calling git clone.
| 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/spridoloba/azure_task_12_deploy_app_with_vm_extention.git |
There was a problem hiding this comment.
This git clone uses a different repository URL and has no explicit target directory. The compute module already clones the repo into /devops_todolist_terraform_task before running this script, so this line is redundant and likely incorrect — remove it or replace it with the same github_repo_url and explicit target devops_todolist_terraform_task to avoid mismatches.
| # 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/spridoloba/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 line assumes a relative path. Make it explicit and robust: either use the absolute path /devops_todolist_terraform_task/app/* (since the module clones there) or use a script-relative path like cp -r "$(dirname "$0")/app"/* /app. Also ensure the source directory exists (after fixing the clone issue).
| @@ -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 the directory already exists.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "prydolobba" |
There was a problem hiding this comment.
The backend configuration uses storage_account_name = "prydolobba" but the task requires the backend to use storage account name yourstorageaccount. Update this to match the task or document why a different account is used and ensure the specified container tfstate exists in that account.
| "zh:d340db7973109761cfc27d52aa02560363337c908b2c99b3628adc5a70a99d5b", | ||
| "zh:d5a577226ebc8c65e8f19384878a86acc4b51ede4b4a82d37c3b331b0efcd4a7", | ||
| "zh:e2962b147f9e71732df8dbc74940c10d20906f3c003cbfaa1eb9fabbf601a9f0", | ||
| "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", |
There was a problem hiding this comment.
Moving /app/todoapp.service will fail if the service file wasn't copied. Add a check that the file exists (e.g., if [ -f /app/todoapp.service ]; then mv ...; else echo "service file missing"; fi) or ensure the earlier copy succeeded before moving.
| network_security_group_name = "defaultnsg" | ||
| public_ip_address_name = "linuxboxpip" | ||
| vm_name = "matebox" | ||
| vm_size = "Standard_B2ts_v2" |
There was a problem hiding this comment.
The task requires the virtual machine size to be Standard_B1s, but here vm_size is set to Standard_B2ts_v2. Change this to Standard_B1s to meet the requirement.
| } | ||
|
|
||
| output "storage_account_name" { | ||
| value = module.storage.storage_account_name |
There was a problem hiding this comment.
The task specification requires the VM size to be Standard_B1s, but here vm_size is set to Standard_B2ts_v2. Change this value to Standard_B1s so the deployment meets the requirement.
| variable "vm_size" { | ||
| type = string | ||
| default = "Standard_B2ts_v2" |
There was a problem hiding this comment.
The task requires the VM size default to be Standard_B1s, but this variable's default is Standard_B2ts_v2. Change the default to Standard_B1s so the deployment matches the specification (checklist/requirements).
| variable "ssh_key_public" { | ||
| type = string | ||
| sensitive = true |
There was a problem hiding this comment.
The ssh_key_public variable is declared but has no default. The task's variable list requires ssh_key_public default to be your-public-key-content. Add default = "your-public-key-content" (and keep sensitive = true) or ensure this value is provided via terraform.tfvars, per the requirements.
https://github.com/mate-academy/devops_todolist_terraform_task/actions/runs/24251815367