dev#60
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: NFR]
Additional info:
Ignored files list:
backend.tfmain.tfmodules/compute/main.tfmodules/compute/outputs.tfmodules/compute/variables.tfmodules/network/main.tfmodules/network/outputs.tfmodules/network/variables.tfmodules/storage/main.tfmodules/storage/outputs.tfmodules/storage/variables.tfoutputs.tfterraform.tfvarsvariables.tf
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because the submission is critically incomplete: the modules/network/main.tf, modules/compute/main.tf, and modules/storage/main.tf files — which contain the actual Azure resource definitions — are missing. Without these, no virtual network, VM, NIC, public IP, NSG, storage account, or task-artifacts container will be created, directly violating core task requirements. Additionally, the backend storage account name in backend.tf is set to mystsddttorage123 instead of the required yourstorageaccount, and terraform.tfvars is absent — both prevent correct Terraform execution. The file() call in modules/compute/main.tf (if it existed) would also fail due to incorrect path resolution for install-app.sh, and the public IP in modules/network/main.tf must use allocation_method = "Dynamic" with sku = "Basic", not Static. Please add all missing main.tf implementations, fix the storage account name, provide terraform.tfvars, and correct the script path and IP configuration.
✨ 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
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "commandToExecute": "bash -c 'echo \"${base64encode(file("install-app.sh"))}\" | base64 --decode > install-app.sh && bash install-app.sh'" |
There was a problem hiding this comment.
The file() function resolves paths relative to the current module's directory, which is modules/compute/. If install-app.sh is in the root directory of your project, this will fail. To correctly reference the script from the root, you should use path.root, like so: file("${path.root}/install-app.sh").
| name = var.pip_name | ||
| location = var.location | ||
| resource_group_name = var.rg_name | ||
| allocation_method = "Static" |
There was a problem hiding this comment.
The task description requires the public IP address to use a dynamic allocation method, but it's currently set to Static. To use a Dynamic allocation, the sku of the public IP address must be set to Basic, not Standard.
| variable "rg_name" {} | ||
| variable "location" {} | ||
| variable "subnet_id" {} | ||
| variable "public_ip_id" {} |
There was a problem hiding this comment.
The backend storage_account_name is set to 'mystsddttorage123' but the task requirement specifies 'yourstorageaccount'. Either update to match or confirm this is intentional.
| variable "rg_name" {} | ||
| variable "location" {} | ||
| variable "subnet_id" {} | ||
| variable "public_ip_id" {} | ||
| variable "vm_name" {} | ||
| variable "vm_size" {} | ||
| variable "ssh_key_public" {} |
There was a problem hiding this comment.
The compute module variables.tf is missing a dns_label variable which might be needed for VM extension or other configuration. Verify if this is required based on the compute module's main.tf implementation.
| output "subnet_id" { value = azurerm_subnet.subnet.id } | ||
| output "public_ip_id" { value = azurerm_public_ip.pip.id } |
There was a problem hiding this comment.
CRITICAL: modules/storage/main.tf is missing. This should create: storage account and container 'task-artifacts'.
| 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" {} |
There was a problem hiding this comment.
All required variables are defined in variables.tf (location, resource_group_name, virtual_network_name, vnet_address_prefix, subnet_name, subnet_address_prefix, network_security_group_name, public_ip_address_name, vm_name, vm_size, ssh_key_public, dns_label). However, default values should be added to match the task requirements: location='uksouth', resource_group_name='mate-azure-task-12', virtual_network_name='vnet', vnet_address_prefix='10.0.0.0/16', subnet_name='default', subnet_address_prefix='10.0.0.0/24', network_security_group_name='defaultnsg', public_ip_address_name='linuxboxpip', vm_name='matebox', vm_size='Standard_B1s', dns_label='matetask'.
| 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" {} |
There was a problem hiding this comment.
CRITICAL: modules/network/main.tf is still missing - should create vnet (vnet, 10.0.0.0/16), subnet (default, 10.0.0.0/24), NSG (defaultnsg), and public IP (linuxboxpip with dynamic allocation and dns_label).
| 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" {} |
There was a problem hiding this comment.
CRITICAL: modules/compute/main.tf is still missing - should create NIC (matebox-nic), VM (matebox, Ubuntu2204, Standard_B1s), and CustomScript extension for install-app.sh.
| 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" {} |
There was a problem hiding this comment.
CRITICAL: modules/storage/main.tf is still missing - should create storage account and container 'task-artifacts'.
| 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" {} |
There was a problem hiding this comment.
terraform.tfvars file is missing - should provide actual values for the deployment.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on implementing all the required modules! The overall structure is correct and very close to completion. However, this requires a change because of a critical configuration error that was also present in the last review.
In backend.tf, the storage_account_name is set to mystsddttorage123, but the task requires it to be yourstorageaccount. This is the main blocker preventing the remote state from being configured correctly.
Additionally, I've left a suggestion in modules/compute/main.tf to use ${path.root} instead of ${path.cwd} for a more robust script path. Please fix the critical backend issue to complete the task. You're almost there!
✨ 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 = "mystsddttorage123" |
There was a problem hiding this comment.
According to the task requirements, the storage_account_name for the remote state backend must be yourstorageaccount. Please update this value to match the specification.
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "commandToExecute": "bash -c 'cat <<EOF > install-app.sh\n${file("${path.cwd}/install-app.sh")}\nEOF\n bash install-app.sh'" |
There was a problem hiding this comment.
While using path.cwd might work when you run terraform from the project's root directory, it's not the most robust approach. A better practice is to use path.root, which always refers to the path of the root module, regardless of the current working directory. This makes your configuration more reliable.
Consider changing ${path.cwd} to ${path.root}.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on fixing the critical backend configuration! The project is now fully functional and very well-structured. To help you keep moving forward, I am approving this submission. However, there are a couple of small but important improvements to consider for your future projects. In modules/compute/main.tf, the script path uses path.cwd, which can be unreliable. A more robust approach is to use path.root to ensure the script is always found. Additionally, the automated checks failed due to formatting; you can easily fix this by running terraform fmt -recursive. Great job overall, you're very close to a perfect solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| settings = <<SETTINGS | ||
| { | ||
| "commandToExecute": "bash -c 'cat <<EOF > install-app.sh\n${file("${path.cwd}/install-app.sh")}\nEOF\n bash install-app.sh'" |
There was a problem hiding this comment.
Using path.cwd here is not robust as it depends on the directory from which Terraform is executed. To ensure the install-app.sh script is always found, please use path.root, which consistently refers to the root directory of your Terraform configuration.
2 participants
https://github.com/goldbergnik1993-cloud/fit-view-k8s/actions/runs/24039274483