Dev#56
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 structure with well-organized modules. However, several key requirements are not met, which prevents the infrastructure from being deployed as specified.
Here are the changes needed:
- Resource Group: In
main.tf, the resource group should be created with aresource "azurerm_resource_group"block, not fetched with adatasource. - Configuration Values: The
terraform.tfvarsfile needs to be updated. Thelocationmust be"uksouth"and thevm_sizemust be"Standard_B1s". - Public IP: The public IP in
modules/network/main.tfmust useDynamicallocation instead ofStatic. This will also require changing theskutoBasicor removing it. - Deployment Script: The
install-app.shscript has a path mismatch. Thecpcommand's source directory must match the directory created by thegit clonecommand. - Backend: The
storage_account_nameinbackend.tfneeds to be updated to"yourstorageaccount"as per the task instructions.
Please address these points to align the configuration with the task requirements.
✨ 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
| resource "azurerm_resource_group" "rg" { | ||
| name = var.resource_group_name | ||
| location = var.location |
There was a problem hiding this comment.
The task requires creating all necessary resources, including the resource group. This data block fetches an existing resource group instead of creating a new one. You should use an azurerm_resource_group resource block to create the group managed by this Terraform configuration.
|
|
||
| backend "azurerm" { | ||
| resource_group_name = "mate-azure-task-12" | ||
| storage_account_name = "dimak300" |
There was a problem hiding this comment.
According to the task requirements, the storage_account_name for the backend should be yourstorageaccount. Please update this value to match the instructions.
| git clone https://github.com/dimak20/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 git clone command creates a directory based on the repository name, which is azure_task_12_deploy_app_with_vm_extention. However, the cp command on the next line references a different directory, devops_todolist_terraform_task. This path mismatch will cause the script to fail because the source directory won't be found.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| network_interface_ids = [azurerm_network_interface.nic.id] | ||
| vm_size = var.vm_size |
There was a problem hiding this comment.
The task requirements specify that the virtual machine size must be Standard_B1s. While using a variable here is good practice, the value provided in your terraform.tfvars file is Standard_D2s_v3, which does not meet the requirement.
| storage_image_reference { | ||
| publisher = "Canonical" | ||
| offer = "0001-com-ubuntu-server-jammy" | ||
| sku = "22_04-lts" | ||
| version = "latest" | ||
| } |
There was a problem hiding this comment.
The requirement is to use an Ubuntu2204 image. While this image reference works, you are using a non-standard offer and SKU. For better compatibility and to follow best practices, consider using the official marketplace image with offer = "UbuntuServer" and sku = "22.04-LTS".
| allocation_method = "Static" | ||
| sku = "Standard" |
There was a problem hiding this comment.
The task requires the public IP address to use a dynamic allocation method. Please change this from Static to Dynamic to meet the requirement. Additionally, the Standard SKU only supports Static allocation. You will need to either remove the sku line (which defaults to Basic, supporting dynamic allocation) or explicitly set it to Basic.
| @@ -0,0 +1,14 @@ | |||
| location = "westeurope" | |||
There was a problem hiding this comment.
The task description specifies that the location should be uksouth. Please update this value.
| network_security_group_name = "defaultnsg" | ||
| public_ip_address_name = "linuxboxpip" | ||
| vm_name = "matebox" | ||
| vm_size = "Standard_D2s_v3" |
There was a problem hiding this comment.
According to the task requirements, the virtual machine size (vm_size) should be Standard_B1s. The current value does not meet this requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! The project is very close to completion, but there are two critical issues that need to be fixed before this can be approved.
-
Invalid Public IP Configuration: In
modules/network/main.tf, theazurerm_public_ipresource is configured withsku = "Standard"andallocation_method = "Dynamic". This is an invalid combination in Azure and will cause the deployment to fail. To fix this, please change theskuto"Basic", which is compatible with dynamic allocation. -
Incorrect Deployment Script Path: The
cpcommand ininstall-app.shwill fail because the source path is incorrect. Thegit clonecommand creates a directory, so you need to update the path to include it, for example:cp -r azure_task_12_deploy_app_with_vm_extention/devops_todolist_terraform_task/app/* /app.
Once you've addressed these two points, the deployment should succeed. Keep up the great work!
✨ 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
| # 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/dimak20/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 copy command will fail because the source path is incorrect. The git clone command creates a directory named azure_task_12_deploy_app_with_vm_extention, and the devops_todolist_terraform_task directory is inside it. The path needs to be updated to reflect this structure, for example: azure_task_12_deploy_app_with_vm_extention/devops_todolist_terraform_task/app/*.
| location = var.location | ||
| resource_group_name = var.resource_group_name | ||
| allocation_method = "Dynamic" | ||
| sku = "Standard" |
There was a problem hiding this comment.
This configuration will cause an error during deployment. A public IP with a Dynamic allocation method cannot have a Standard SKU. According to the task requirements and Azure constraints, you should use the Basic SKU for dynamic allocation.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this project! You've successfully addressed all the previous feedback, and the Terraform configuration now perfectly aligns with all the task requirements. The project is well-structured, uses modules effectively, and demonstrates a solid understanding of deploying infrastructure on Azure. I am happy to approve your submission. 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.