Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes and automates deployment for the solution by introducing an azd-driven workflow (pre/post-provision hooks), container-based App Service/Function deployments, automated SQL bootstrap, and improved operational health/observability endpoints.
Changes:
- Add
deploy/-basedazddeployment hooks + Bicep refactor into reusable modules for core infra, apps, and hosts. - Automate SQL initialization and Linux host SQL registration during
postprovision, including new schema/proc scripts. - Containerize frontend/API/task, add
/healthendpoints, and enable Azure Monitor OpenTelemetry instrumentation.
Reviewed changes
Copilot reviewed 52 out of 55 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| task/Dockerfile | Adds container build for the scheduled task Function App image. |
| sql_queries/README.md | Updates SQL docs to describe automated bootstrap + manual fallback. |
| sql_queries/025_create_procedure-RegisterLinuxHostVm.sql | Adds proc used to upsert Linux hosts into VirtualMachines. |
| sql_queries/024_create_table-vmusers.sql | Makes VmUsers table creation rerunnable and schema-qualified. |
| sql_queries/003_create_table-virtual_machines.sql | Makes VirtualMachines table creation rerunnable. |
| sql_queries/002_create_table-vm_scaling_activity_log.sql | Makes VmScalingActivityLog creation rerunnable. |
| sql_queries/001_create_table-vm_scaling_rules.sql | Makes VmScalingRules creation rerunnable + seeds default rules if empty. |
| README.md | Points “Getting Started” to the supported deploy/ workflow. |
| linux_host/session_release_buffer/Ubuntu/release-session.sh | Updates API base URL placeholder for scripted replacement. |
| linux_host/session_release_buffer/RHEL/release-session.sh | Updates API base URL placeholder for scripted replacement. |
| front_end/templates/base.html | Removes local CSS reference, relying on CDN bootstrap styling. |
| front_end/route_vm_management.py | Adjusts VM list view behavior when API returns 404. |
| front_end/requirements.txt | Adds Azure Monitor OpenTelemetry dependency. |
| front_end/Dockerfile | Adds container build for the frontend web app. |
| front_end/app.py | Adds Azure Monitor OpenTelemetry + /health endpoint + consistent logger name. |
| deploy/Register-LinuxHostSqlRecords.ps1 | Adds post-provision script to register Linux VMs into SQL via stored procedure. |
| deploy/Post-Provision.ps1 | Adds orchestrated post-provision sequence (build images, init SQL, assign roles, group sync, register hosts). |
| deploy/Initialize-DeploymentEnvironment.ps1 | Adds preprovision bootstrap for Entra apps/groups, secrets, SSH keys, and generated bicep params. |
| deploy/Initialize-Database.ps1 | Adds SQL bootstrap runner (sort scripts, split on GO, rewrite to CREATE OR ALTER, fail fast). |
| deploy/DEPLOYMENT.md | Adds detailed azd deployment guide and troubleshooting. |
| deploy/Build-ContainerImages.ps1 | Adds ACR remote build + app restarts with retry logic. |
| deploy/bicep/modules/Linux/main.bicep | Updates Linux VM module to accept API base URL/client ID; improves SSH vs password config handling. |
| deploy/bicep/modules/core/storage-account.bicep | Adds storage account module used by function app. |
| deploy/bicep/modules/core/sql-database.bicep | Adds SQL server/db + firewall rules module. |
| deploy/bicep/modules/core/observability.bicep | Adds Log Analytics + Application Insights module. |
| deploy/bicep/modules/core/networking.bicep | Adds VNet/subnets module for apps/hosts/private endpoints. |
| deploy/bicep/modules/core/key-vault.bicep | Adds Key Vault + secret creation for DB password and Linux host private key. |
| deploy/bicep/modules/core/container-registry.bicep | Adds ACR module for container build/pull. |
| deploy/bicep/modules/core/app-service-plan.bicep | Adds App Service plan module (Premium v3 baseline). |
| deploy/bicep/modules/AVD/token.bicep | Updates host pool registration token output handling. |
| deploy/bicep/modules/AVD/main.bicep | Minor cleanup and parameter comments adjustments for AVD module. |
| deploy/bicep/modules/apps/container-web-app.bicep | Adds reusable module for container-based App Service with auth/appsettings/logging/health check support. |
| deploy/bicep/modules/apps/container-function-app.bicep | Adds reusable module for container-based Function App with required settings. |
| deploy/bicep/main.resources.bicep | Adds full resource-group-scoped deployment composition and app settings wiring. |
| deploy/bicep/main.parameters.example.json | Adds example parameters template for manual reference. |
| deploy/bicep/main.bicep | Adds subscription-scoped entrypoint that creates RG and invokes main.resources.bicep. |
| deploy/azurecli/Assign-AppRoleToFunctionApp.ps1 | Minor formatting cleanup. |
| deploy/azure.yaml | Adds azd project definition and hook wiring (Windows/pwsh). |
| deploy/Assign-VmApiRoles.ps1 | Adds Entra group membership sync for VM managed identities based on broker-role tag. |
| deploy/Assign-ServicePrincipalApiRole.ps1 | Adds helper to assign API application roles to a service principal via Microsoft Graph. |
| deploy/Assign-FunctionAppApiRole.ps1 | Adds helper to assign the ScheduledTask API role to the function app managed identity. |
| custom_script_extensions/Configure-Ubuntu24_desktop-Host.sh | Requires API base URL/client ID args and performs placeholder replacement. |
| custom_script_extensions/Configure-RHEL9-Host.sh | Requires API base URL/client ID args and performs placeholder replacement. |
| custom_script_extensions/Configure-RHEL8-Host.sh | Requires API base URL/client ID args and performs placeholder replacement. |
| custom_script_extensions/Configure-RHEL7-Host.sh | Requires API base URL/client ID args and performs placeholder replacement. |
| bicep/Linux/README.md | Removes older standalone bicep docs in favor of deploy/ workflow. |
| bicep/Linux/main.bicepparam | Removes older standalone parameter file. |
| bicep/AVD/README.md | Removes older standalone bicep docs in favor of deploy/ workflow. |
| bicep/AVD/main.bicepparam | Removes older standalone parameter file. |
| api/requirements.txt | Adds Azure Monitor OpenTelemetry dependency. |
| api/Dockerfile | Adds container build for the API web app. |
| api/app.py | Adds Azure Monitor OpenTelemetry, /health endpoint, and returns empty VM list as 200. |
| .gitignore | Ignores azd .azure folder and generated bicep parameters with secrets. |
| .dockerignore | Adds repo-level dockerignore to reduce build context and avoid leaking local artifacts. |
Comments suppressed due to low confidence (1)
deploy/bicep/modules/AVD/token.bicep:39
- The host pool registration token is a secret; emitting it as a deployment output makes it available to anyone with deployment-read access and can be captured in logs. Prefer passing the token directly into the VM/DSC extension properties without outputting it, or mark the output as secure and tightly scope who can read deployment outputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response = requests.get(f"{API_URL}/vms", headers=headers) | ||
| if response.status_code == 404: | ||
| return render_template('vm/view_all_vms.html', vms=[]) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Handling a 404 from the API by rendering an empty VM list can mask real misconfiguration (wrong API_URL, missing /api prefix, or route changes). Since the API now returns 200 with an empty array when there are no VMs, consider removing this special-case or only treating 404 as empty when you can positively identify it as the “no VMs” condition (otherwise surface an error to the user).
| SELECT TOP 1 @ExistingVmId = VMID | ||
| FROM dbo.VirtualMachines | ||
| WHERE Hostname = @Hostname | ||
| ORDER BY VMID; | ||
|
|
There was a problem hiding this comment.
This procedure updates only the first row matching Hostname (TOP 1 ORDER BY VMID). Because dbo.VirtualMachines does not enforce Hostname uniqueness, duplicate hostnames will lead to nondeterministic updates and potentially leave stale rows behind. Consider enforcing a UNIQUE constraint on Hostname (or changing the procedure to update by Hostname and handle duplicates explicitly).
| output id string = storageAccount.id | ||
| output name string = storageAccount.name | ||
| output connectionString string = 'DefaultEndpointsProtocol=https;AccountName=${storageAccount.name};AccountKey=${storageAccount.listKeys().keys[0].value};EndpointSuffix=${environment().suffixes.storage}' |
There was a problem hiding this comment.
This module outputs a full storage account connection string containing the account key (listKeys). ARM/Bicep deployment outputs are broadly readable by anyone with deployment read access, so this leaks a secret. Avoid outputting the key-bearing connection string (compute it only where needed and pass as a secure parameter), or at minimum mark the output as secure and ensure it is not logged/printed anywhere.
| USE linuxbroker; | ||
|
|
There was a problem hiding this comment.
These SQL scripts are executed over a connection that already targets the desired database (Initialize-Database.ps1 takes -DatabaseName). Hardcoding USE linuxbroker; makes it easy to accidentally run against the wrong DB name (or fail in environments where the DB name differs / is case-sensitive). Consider removing USE ... lines from the scripts so the deployment parameter controls the target database reliably.
| ActionTaken NVARCHAR(50) NOT NULL, | ||
| VMsPoweredOn INT NULL, | ||
| VMsPoweredOff INT NULL, | ||
| NewTotalVMs INT NOT NULL, | ||
| Outcome NVARCHAR(255) NULL, | ||
| Notes TEXT NULL | ||
| ); |
There was a problem hiding this comment.
TEXT is deprecated in SQL Server/Azure SQL and can cause tooling/type-mapping issues. Since this table is being refreshed to be rerunnable, consider switching Notes to NVARCHAR(MAX) (or VARCHAR(MAX)) for forward compatibility.
No description provided.