solution#45
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your project has a great modular structure, but several critical issues are preventing it from meeting the core requirements. Changes are needed before this can be approved.
Here are the blockers that must be addressed:
- Resource Group Creation: The root
main.tfmust define and create the Azure Resource Group using anazurerm_resource_groupresource. Currently, it relies on a pre-existing one. - Deployment Script URL: The
azurerm_virtual_machine_extensioninmodules/compute/main.tfuses a placeholder URL. You need to replace<твій-юзернейм>and<repo>with your actual GitHub username and repository name so the application can be deployed. - Missing Variable Defaults: The task requires all variables in the root
variables.tfto have specifieddefaultvalues. These are currently missing. - Incorrect Backend Name: In
backend.tf, thestorage_account_nameneeds to be updated toyourstorageaccountto match the task instructions.
✨ 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
| provider "azurerm" { | ||
| features {} | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the task description, the Terraform configuration should create the Azure Resource Group. This resource definition is missing from the root module. You should add an azurerm_resource_group resource here and use its name when calling the modules below.
| terraform { | ||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "meow12345" |
There was a problem hiding this comment.
According to the task requirements, the storage_account_name for the remote backend configuration should be yourstorageaccount. Please update this value to match the instructions.
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "fileUris": ["https://raw.githubusercontent.com/<твій-юзернейм>/<repo>/main/install-app.sh"], |
There was a problem hiding this comment.
The URL for the install-app.sh script is a placeholder. You need to replace <твій-юзернейм> and <repo> with your actual GitHub username and repository name so that the script can be downloaded and executed on the virtual machine.
| 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 state that all variables in the root variables.tf should be defined with default values. Currently, all variables are declared without defaults. Please add the default values as specified in the task description (e.g., default = "uksouth" for the location variable).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! You've successfully addressed all the critical feedback from the previous review, and your Terraform configuration now meets all the core requirements of the task. I am happy to approve your solution.
For future reference, there is a minor improvement you could make regarding the duplicated random_integer resource. It's currently defined in both the root main.tf and the network module. A common best practice is to define such resources only once (in the root module) and pass the value to any modules that need it. This follows the "Don't Repeat Yourself" (DRY) principle and improves maintainability.
This is just a suggestion for improvement and doesn't block the approval. Great job getting this project to a fully functional state!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| resource "random_integer" "rand" { | ||
| min = 1000 | ||
| max = 9999 | ||
| } No newline at end of file |
There was a problem hiding this comment.
This random_integer resource is a good way to ensure unique names. However, the network module also defines its own identical random_integer resource.
To avoid duplication and improve consistency, it's better to define this resource only once here in the root main.tf. You can then pass the result (random_integer.rand.result) as a variable to the network module, just as you've done for the storage module.
| resource "random_integer" "rand" { | ||
| min = 1000 | ||
| max = 9999 | ||
| } |
There was a problem hiding this comment.
This resource correctly generates a random number for the DNS label. However, there's a similar random_integer resource defined in the root main.tf.
To avoid duplication and centralize the random number generation, it's a best practice to remove this resource from the module. You can add a new variable to modules/network/variables.tf to accept the random number and pass it in from the root main.tf.
| variable "subnet_address_prefix" {} | ||
| variable "network_security_group_name" {} | ||
| variable "public_ip_address_name" {} | ||
| variable "dns_label" {} No newline at end of file |
There was a problem hiding this comment.
To follow the best practice of defining a resource only once, consider adding a new variable here to receive the random number generated in the root main.tf. For example:
variable "random_suffix" {
type = number
}You could then pass random_integer.rand.result from the root module to this new variable and remove the duplicate random_integer resource from this module's main.tf.
No description provided.