Skip to content

Azure SQL Database configuration parameters and module#274

Closed
ramirezmorac2 wants to merge 3 commits intodevelopfrom
create-sqldatabase-2
Closed

Azure SQL Database configuration parameters and module#274
ramirezmorac2 wants to merge 3 commits intodevelopfrom
create-sqldatabase-2

Conversation

@ramirezmorac2
Copy link
Copy Markdown
Collaborator

JIRA Ticket

PROJ-XXX

Description

[Describe your changes here]

Checklist

  • Code review requested
  • Tests completed
  • Documentation updated

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds Azure SQL Database infrastructure configuration to the deployment template. The changes introduce parameters for SQL Server configuration including server name, database name, administrator credentials, and network access settings, along with a new module deployment for SQL Server with Key Vault integration.

Changes:

  • Added six new parameters for Azure SQL configuration (server name, database name, admin credentials, secret name, network access)
  • Integrated SQL Server module deployment with Key Vault for secret storage
  • Added output variables for SQL Server and Database names
  • Minor formatting fixes for spacing consistency

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
infra/main.parameters.json Added SQL Server configuration parameters with environment variable mappings and empty defaults for server/database names
infra/main.bicep Added SQL Server parameter declarations, variable definitions, module deployment, output variables, and minor spacing fixes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

scope: resourceGroup
params: {
name: sqlServerName
location: 'eastus2'
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hard-coded location 'eastus2' may cause deployment issues if the primary deployment location specified in the location parameter is different. This creates a potential mismatch where the SQL Server is deployed to a different region than other resources.

While hard-coded locations are used in this codebase for specific services with regional availability constraints (e.g., visionIngestion uses 'westus', o1Deployment uses 'eastus2'), consider whether this SQL Server deployment has similar constraints. If not, using the location parameter would provide more flexibility and keep resources colocated.

If the hard-coded location is intentional due to service availability, add a comment explaining the reason (e.g., '//Workaround for service availability') to maintain consistency with the existing pattern at line 897.

Suggested change
location: 'eastus2'
location: location

Copilot uses AI. Check for mistakes.
param azureSqlDatabaseName string = ''
var sqlDatabaseName = !empty(azureSqlDatabaseName) ? azureSqlDatabaseName : 'sqldb0-${resourceToken}'
@description('Azure SQL administrator login name.')
param azureSqlAdministratorLogin string = ''
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The azureSqlAdministratorLogin parameter defaults to an empty string but is required by the SQL Server resource. If the AZURE_SQL_ADMIN_LOGIN environment variable is not set, this will cause a deployment failure.

Consider either:

  1. Providing a generated default value (e.g., 'sqladmin-${resourceToken}') to match the pattern used for other resources, or
  2. Removing the default value to make it explicitly required, which would cause validation to fail early with a clear error message

This prevents silent failures during deployment and improves the developer experience.

Suggested change
param azureSqlAdministratorLogin string = ''
param azureSqlAdministratorLogin string = 'sqladmin-${resourceToken}'

Copilot uses AI. Check for mistakes.
@secure()
param azureSqlAdministratorPassword string
@description('Key Vault secret name to store the Azure SQL administrator password.')
param azureSqlAdminSecretName string = ''
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The azureSqlAdminSecretName parameter defaults to an empty string but is required for storing the SQL administrator password in Key Vault. If the AZURE_SQL_ADMIN_SECRET_NAME environment variable is not set, this will cause a deployment failure when the sqlserver module attempts to create the Key Vault secret.

Consider either:

  1. Providing a generated default value (e.g., 'sql-admin-password') to match the pattern used for other secret names in the codebase (see 'azureDBkey' at line 913, 'azureOpenAIKey' at line 1868, 'azureSearchKey' at line 1927), or
  2. Removing the default value to make it explicitly required

This prevents silent failures during deployment and maintains consistency with how other database secrets are named.

Suggested change
param azureSqlAdminSecretName string = ''
param azureSqlAdminSecretName string = 'sql-admin-password'

Copilot uses AI. Check for mistakes.
Comment on lines +975 to +989
module sqlServer './core/db/sqlserver.bicep' = {
name: 'sqlserver'
scope: resourceGroup
params: {
name: sqlServerName
location: 'eastus2'
tags: tags
administratorLogin: azureSqlAdministratorLogin
administratorLoginPassword: azureSqlAdministratorPassword
databaseName: sqlDatabaseName
keyVaultName: keyVault.outputs.name
publicNetworkAccess: azureSqlPublicNetworkAccess
secretName: azureSqlAdminSecretName
}
}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SQL Server module is deployed but the connection information (server name, database name, connection string) is not being passed to any application components (orchestrator, frontend, dataIngestion, mcpServer).

The outputs AZURE_SQL_SERVER_NAME and AZURE_SQL_DATABASE_NAME are exposed at lines 2155-2156, but no application settings are configured to use these values. This suggests the SQL Server may not be fully integrated.

If this SQL Server is intended to be used by the application, consider adding the necessary connection information to the appSettings of the relevant modules. For example:

  • SQL_SERVER_NAME or DATABASE_SERVER
  • SQL_DATABASE_NAME or DATABASE_NAME
  • SQL_CONNECTION_STRING (retrieving the password from Key Vault)

If the SQL Server is not yet intended to be used and this is preparatory infrastructure, consider making the module deployment conditional to avoid unnecessary resource costs.

Copilot uses AI. Check for mistakes.
Comment on lines +975 to +989
module sqlServer './core/db/sqlserver.bicep' = {
name: 'sqlserver'
scope: resourceGroup
params: {
name: sqlServerName
location: 'eastus2'
tags: tags
administratorLogin: azureSqlAdministratorLogin
administratorLoginPassword: azureSqlAdministratorPassword
databaseName: sqlDatabaseName
keyVaultName: keyVault.outputs.name
publicNetworkAccess: azureSqlPublicNetworkAccess
secretName: azureSqlAdminSecretName
}
}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When network isolation is enabled, other database resources (like Cosmos DB at line 918) have private endpoints configured with corresponding DNS zones. However, no private endpoint is configured for the SQL Server.

If network isolation is a feature that should apply to all database services consistently, consider adding:

  1. A SQL DNS zone module (similar to blobDnsZone, documentsDnsZone, etc.)
  2. A private endpoint module for SQL Server (similar to cosmospe at line 918)

This ensures consistent network security across all database resources when networkIsolation is enabled. If SQL Server doesn't require network isolation by design, the publicNetworkAccess parameter can handle this, but the inconsistency with other databases should be documented.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +57
"value": ""
},
"azureSqlDatabaseName": {
"value": ""
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The azureSqlServerName and azureSqlDatabaseName parameters have empty string values, which differs from the pattern used for other similar resources in this file.

Compare with:

  • azureDbAccountName (line 47-48): uses environment variable ${AZURE_DB_ACCOUNT_NAME}
  • azureStorageAccountName (line 80-81): uses environment variable ${AZURE_STORAGE_ACCOUNT_NAME}
  • azureCognitiveServiceName (line 83-84): uses environment variable ${AZURE_COGNITIVE_SERVICE_NAME}

Consider using environment variables like ${AZURE_SQL_SERVER_NAME} and ${AZURE_SQL_DATABASE_NAME} to maintain consistency with other resource naming patterns. This allows users to specify custom names when needed, while the bicep file's default value generation (lines 411 and 414 in main.bicep) will be used when the environment variables are empty.

Suggested change
"value": ""
},
"azureSqlDatabaseName": {
"value": ""
"value": "${AZURE_SQL_SERVER_NAME}"
},
"azureSqlDatabaseName": {
"value": "${AZURE_SQL_DATABASE_NAME}"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants