Conversation
chore: Update post-release process steps and fix deployment issues
…esh authentication token
…eck from deployment workflow
…ID for Service Principal
…onment if not provided
…ross documentation
fix: add bicep version requirement (>= 0.33.0) to azure.yaml
ci: Enhance Azure Dev workflow with submodule support and service principal configuration
fix: Set functional default VM admin password and credential params
feat: Add quota check scripts for Azure OpenAI models and Fabric capacity
Saswato-Microsoft
requested review from
Avijit-Microsoft,
Prajwal-Microsoft,
Roopan-Microsoft,
Vinay-Microsoft,
aniaroramsft and
dgp10801
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves deployment reliability and documentation by adding pre-deployment quota checking scripts, tightening CI workflow triggers, and standardizing infrastructure parameters for principal handling and VM credentials.
Changes:
- Added Bash and PowerShell quota check scripts and expanded quota documentation (OpenAI + optional Fabric capacity checks).
- Updated GitHub Actions workflows to reduce unnecessary runs, checkout submodules recursively, and automate principal ID/resource group handling.
- Updated Bicep parameters and docs to standardize principal type and Jump VM credential configuration.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/quota_check.sh | Adds Bash quota checker for OpenAI (and optional Fabric) across regions. |
| scripts/quota_check.ps1 | Adds PowerShell quota checker equivalent to the Bash script. |
| infra/main.bicepparam | Adds env-driven principal type and VM username; changes VM password default. |
| docs/quota_check.md | Updates quota-check guidance and adds Bash/PowerShell usage examples + sample output. |
| docs/post_deployment_steps.md | Updates Bastion/Jump VM credential instructions and defaults. |
| docs/deploymentguide.md | Updates VM credential configuration guidance and examples. |
| docs/deploy_app_from_foundry.md | Clarifies where VM credentials come from. |
| docs/ACCESSING_PRIVATE_RESOURCES.md | Updates Jump VM credential retrieval/reset guidance. |
| azure.yaml | Adds required Bicep version constraint. |
| .github/workflows/azure-dev.yml | Adds submodule checkout + principal ID resolution + RG creation before provisioning. |
| .github/workflows/azd-template-validation.yml | Adds path filters, submodule checkout, and principal env vars for validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| param vmAdminPassword = readEnvironmentVariable('VM_ADMIN_PASSWORD', '$(secretOrRandomPassword)') | ||
| param vmUserName = readEnvironmentVariable('VM_ADMIN_USERNAME', 'testvmuser') | ||
| param vmAdminPassword = readEnvironmentVariable('VM_ADMIN_PASSWORD', 'JumpboxAdminP@ssw0rd1234!') |
There was a problem hiding this comment.
Hardcoding a default admin password (even as a fallback) is a severe security risk: it creates a known credential that may be deployed unintentionally. Prefer keeping the prior secret-or-random default (or requiring the value) and only overriding via VM_ADMIN_PASSWORD; if you want a deterministic dev default, keep it out of source control (e.g., set in a local .env/azd env) and document it as an example placeholder, not an actual default.
| param vmAdminPassword = readEnvironmentVariable('VM_ADMIN_PASSWORD', 'JumpboxAdminP@ssw0rd1234!') | |
| param vmAdminPassword = readEnvironmentVariable('VM_ADMIN_PASSWORD', '') |
| If you prefer source-controlled defaults, set them in [infra/main.bicepparam](../infra/main.bicepparam) instead: | ||
|
|
||
| ```bicep | ||
| param vmUserName = 'youradminuser' | ||
| param vmAdminPassword = 'Use-A-Strong-Password-Here!' |
There was a problem hiding this comment.
This guidance encourages committing VM admin credentials to source control. Please change the doc to recommend using azd env set (or a secrets manager / pipeline secret variables) and keep the Bicep snippet as a non-secret placeholder (e.g., show only vmUserName, and for password show a reference to readEnvironmentVariable / secretOrRandomPassword), explicitly warning not to commit passwords.
| If you prefer source-controlled defaults, set them in [infra/main.bicepparam](../infra/main.bicepparam) instead: | |
| ```bicep | |
| param vmUserName = 'youradminuser' | |
| param vmAdminPassword = 'Use-A-Strong-Password-Here!' | |
| Use `azd env set`, a secrets manager, or pipeline secret variables for the password. Do **not** commit VM passwords to source control. | |
| If you need a non-secret placeholder in [infra/main.bicepparam](../infra/main.bicepparam), keep the username only and reference the password from the environment: | |
| ```bicep | |
| param vmUserName = 'youradminuser' | |
| param vmAdminPassword = readEnvironmentVariable('VM_ADMIN_PASSWORD', '') |
| - name: Create Resource Group if needed | ||
| run: | | ||
| # Use provided RG name or derive from environment name | ||
| RESOURCE_GROUP="${AZURE_RESOURCE_GROUP:-rg-${AZURE_ENV_NAME}}" | ||
| echo "Using resource group: $RESOURCE_GROUP" | ||
|
|
||
| RG_EXISTS=$(az group exists --name "$RESOURCE_GROUP") | ||
| if [ "$RG_EXISTS" = "false" ]; then | ||
| echo "Creating resource group: $RESOURCE_GROUP" | ||
| az group create --name "$RESOURCE_GROUP" --location ${{ vars.AZURE_LOCATION }} | ||
| else | ||
| echo "Resource group already exists: $RESOURCE_GROUP" | ||
| fi | ||
|
|
||
| # Set for subsequent steps | ||
| echo "RESOURCE_GROUP=$RESOURCE_GROUP" >> $GITHUB_ENV | ||
|
|
||
| - name: Provision Infrastructure | ||
| id: provision-main | ||
| run: azd provision --no-prompt |
There was a problem hiding this comment.
This step computes/creates a resource group but exports it as RESOURCE_GROUP, while the job/environment uses AZURE_RESOURCE_GROUP. If AZURE_RESOURCE_GROUP is empty, azd provision likely won't pick up RESOURCE_GROUP and may provision into a different RG than the one you just created. Export the derived value back into AZURE_RESOURCE_GROUP (or whatever azd expects) so the provisioning step uses the same RG.
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| TEMP: /tmp | ||
| fabricCapacityMode: 'none' | ||
| AZURE_PRINCIPAL_ID: ${{ vars.PRINCIPAL_ID || secrets.AZURE_CLIENT_ID }} |
There was a problem hiding this comment.
Falling back to secrets.AZURE_CLIENT_ID for AZURE_PRINCIPAL_ID is likely incorrect: client/app ID is not the service principal object ID, and role assignments typically require the object ID. This can cause RBAC assignment failures during validation. Align this workflow with the object-ID resolution logic used in azure-dev.yml, or require an explicit PRINCIPAL_ID/AZURE_PRINCIPAL_ID value.
| AZURE_PRINCIPAL_ID: ${{ vars.PRINCIPAL_ID || secrets.AZURE_CLIENT_ID }} | |
| AZURE_PRINCIPAL_ID: ${{ vars.PRINCIPAL_ID || secrets.AZURE_PRINCIPAL_ID }} |
| // Entra object ID of the identity to grant RBAC (user, group, service principal, or UAI). Set this if Graph lookup is blocked. | ||
| param principalId = readEnvironmentVariable('AZURE_PRINCIPAL_ID', '') | ||
| param principalType = 'User' | ||
| param principalType = readEnvironmentVariable('AZURE_PRINCIPAL_TYPE', 'User') |
There was a problem hiding this comment.
PR description says AZURE_PRINCIPAL_TYPE defaults to 'ServicePrincipal', but the Bicep parameter defaults to 'User'. Please align the default behavior with the documented intent (either update the Bicep default, or adjust the PR description/docs if 'User' is the intended default).
| echo "🔍 Checking region: $REGION" | ||
|
|
||
| ALL_PASS=true | ||
| safe_region="${REGION//[^a-zA-Z0-9]/_}" |
There was a problem hiding this comment.
Using eval for result storage/lookups creates an avoidable code-injection footgun and makes the script harder to maintain. Prefer storing results in an associative array keyed by something like "${REGION}:${i}" (or two-level associative arrays) so you can set/get values without eval.
| echo " (Looked for: $primary_key${alt_key:+, $alt_key})" | ||
| fi | ||
| ALL_PASS=false | ||
| eval "RESULT_${safe_region}_${i}=N_A" |
There was a problem hiding this comment.
Using eval for result storage/lookups creates an avoidable code-injection footgun and makes the script harder to maintain. Prefer storing results in an associative array keyed by something like "${REGION}:${i}" (or two-level associative arrays) so you can set/get values without eval.
| LIMIT=${LIMIT%%.*} | ||
| AVAILABLE=$((LIMIT - CURRENT)) | ||
|
|
||
| eval "RESULT_${safe_region}_${i}=${AVAILABLE}_${LIMIT}" |
There was a problem hiding this comment.
Using eval for result storage/lookups creates an avoidable code-injection footgun and makes the script harder to maintain. Prefer storing results in an associative array keyed by something like "${REGION}:${i}" (or two-level associative arrays) so you can set/get values without eval.
|
|
||
| for ((i=0; i<MODEL_COUNT; i++)); do | ||
| mcap="${MODEL_CAPS[$i]}" | ||
| eval "val=\${RESULT_${safe_region}_${i}:-N_A}" |
There was a problem hiding this comment.
Using eval for result storage/lookups creates an avoidable code-injection footgun and makes the script harder to maintain. Prefer storing results in an associative array keyed by something like "${REGION}:${i}" (or two-level associative arrays) so you can set/get values without eval.
| eval "val=\${RESULT_${safe_region}_${i}:-N_A}" | |
| result_var="RESULT_${safe_region}_${i}" | |
| val="${!result_var:-N_A}" |
| - name: Resolve Service Principal Object ID | ||
| run: | | ||
| # If PRINCIPAL_ID repo variable is set and is a valid GUID, use it directly | ||
| if [[ "${{ vars.PRINCIPAL_ID }}" =~ ^[0-9a-fA-F-]{36}$ ]]; then |
There was a problem hiding this comment.
The GUID check regex is overly permissive (it allows any mix of hex and hyphens of length 36, even with hyphens in the wrong positions). Consider using a stricter pattern (8-4-4-4-12) to avoid accidentally accepting invalid IDs and then failing later during azd provision.
| if [[ "${{ vars.PRINCIPAL_ID }}" =~ ^[0-9a-fA-F-]{36}$ ]]; then | |
| if [[ "${{ vars.PRINCIPAL_ID }}" =~ ^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$ ]]; then |
Purpose
This pull request introduces several improvements and clarifications to the deployment process, infrastructure configuration, and documentation for the accelerator. The main changes enhance automation for service principal handling, clarify and standardize VM credential management, update quota check instructions, and improve workflow triggers and environment variable handling for CI/CD. These updates aim to make deployments more reliable, transparent, and user-friendly.
Infrastructure and Workflow Automation:
AZURE_PRINCIPAL_IDdynamically if not provided, and useAZURE_PRINCIPAL_TYPEwith a default of'ServicePrincipal'. This ensures correct role assignments and RBAC configuration during deployment. (.github/workflows/azure-dev.yml,infra/main.bicepparam, [1] [2] [3].github/workflows/azd-template-validation.yml,.github/workflows/azure-dev.yml, [1] [2] [3]VM Credential Management and Documentation:
VM_ADMIN_USERNAME,VM_ADMIN_PASSWORD) are now the recommended method, with clear fallbacks and defaults (testvmuserfor username, a default password). Documentation is updated to reflect this across guides and troubleshooting sections. (infra/main.bicepparam,docs/ACCESSING_PRIVATE_RESOURCES.md,docs/deploymentguide.md,docs/post_deployment_steps.md, [1] [2] [3] [4] [5]Quota Check and Model/Capacity Documentation:
docs/quota_check.md, [1] [2]Other Notable Improvements:
azure.yamlto ensure compatibility. (azure.yaml, azure.yamlR5).github/workflows/azure-dev.yml, .github/workflows/azure-dev.ymlL27-R96)Let me know if you want to walk through any of these changes in detail or see how they impact your development workflow!
Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify that the following are valid
Other Information