terraform task#59
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your project has a great modular structure, which is excellent! However, I'm requesting a few changes to address some critical issues that prevent the deployment from succeeding and meeting all requirements.
Here are the key items to fix:
- Resource Group Creation: The configuration is missing the
azurerm_resource_groupresource inmain.tf. The task requires Terraform to create all resources, so please add this block. - Application Inaccessibility: The network security group in
modules/network/main.tfneeds a security rule to allow inbound HTTP traffic on port 80. Without this, the ToDo application won't be accessible. - VM Extension Script: The
azurerm_virtual_machine_extensioninmodules/compute/main.tfis missing a source forinstall-app.sh. You need to provide the script's location using thefile_urisproperty. - Backend Configuration: In
backend.tf, thestorage_account_namemust be set toyourstorageaccountas specified in the requirements.
Once these issues are resolved, your configuration should be fully functional.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "gosxanstorage123" |
There was a problem hiding this comment.
According to the task requirements, the storage_account_name should be set to yourstorageaccount. Please update this value to match the specification.
There was a problem hiding this comment.
The storage account name 'yourstorageaccount' is already taken.
| provider "azurerm" { | ||
| features {} | ||
| } | ||
|
|
There was a problem hiding this comment.
The task description requires the Terraform configuration to create all resources, including the resource group. You're correctly passing the resource group name to the modules, but the azurerm_resource_group resource definition is missing. Please add the resource block for the resource group in this file.
| resource "azurerm_network_security_group" "nsg" { | ||
| name = var.network_security_group_name | ||
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| } |
There was a problem hiding this comment.
This network security group is created without any security rules. To allow users to access the ToDo list application on the virtual machine, you need to add an inbound security rule to permit HTTP traffic on port 80 from the internet (source_address_prefix = "Internet").
| settings = jsonencode({ | ||
| commandToExecute = "bash install-app.sh" | ||
| }) |
There was a problem hiding this comment.
The azurerm_virtual_machine_extension is configured to execute bash install-app.sh, but the configuration does not specify how this script file will be made available to the virtual machine. The CustomScript extension needs a source for the script. You can provide this using the file_uris property within the settings block. This property should contain a URL where the script is hosted (e.g., Azure Blob Storage or a public Git repository).
| variable "location" {} | ||
| variable "resource_group_name" {} | ||
|
|
||
| variable "virtual_network_name" {} | ||
| variable "vnet_address_prefix" {} | ||
|
|
||
| variable "subnet_name" {} | ||
| variable "subnet_address_prefix" {} | ||
|
|
||
| variable "network_security_group_name" {} | ||
| variable "public_ip_address_name" {} | ||
|
|
||
| variable "vm_name" {} | ||
| variable "vm_size" {} | ||
| variable "ssh_key_public" {} | ||
|
|
||
| variable "dns_label" {} No newline at end of file |
There was a problem hiding this comment.
The task requirements specify exact values for most of these variables (e.g., location = "uksouth"). It's a good practice to define these as default values directly in the variable blocks. This makes the configuration self-contained and ensures it uses the correct parameters even if a .tfvars file is missing. The only variable that should not have a default value here is ssh_key_public, as that is user-specific.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this task! You've addressed most of the previous feedback, and the modular structure is looking great. However, I am requesting changes because a couple of critical requirements are still not met.
The most important issue is in backend.tf. The storage_account_name is currently set to gosxanstorage123, but the task explicitly requires it to be yourstorageaccount. This is a critical requirement for the remote state configuration.
Additionally, the project structure requires an outputs.tf file in the root directory, which is currently missing. Please add this file to complete the setup. Once these two items are fixed, we should be ready to go!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "gosxanstorage123" |
There was a problem hiding this comment.
The task requirements specify that the storage_account_name for the remote backend should be yourstorageaccount. Please update this value to match the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work addressing all the previous feedback! You've correctly added the missing outputs.tf files and fixed the backend configuration, which is fantastic. I am approving your solution as an educational override so you can move forward, but please note that an automated check found a minor formatting issue. The file modules/compute/main.tf has some incorrect indentation. You can easily fix this for future projects by running terraform fmt -recursive from your root directory. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.