-
Notifications
You must be signed in to change notification settings - Fork 79
feat: Code-modernize: Add support for developer experience #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 PR introduces custom deployment configuration files to enable local code deployment alongside the existing deployment mechanism. The changes add Docker containerization for frontend and backend services, comprehensive Bicep infrastructure templates, and updated deployment documentation.
Key Changes:
- Multi-stage Dockerfiles for containerized frontend (Node.js build + Python runtime) and backend (Python) services
- Comprehensive Bicep infrastructure template (
main_custom.bicep) with 864 lines defining Azure resources including AI services, Container Apps, Cosmos DB, Storage, Key Vault, and optional private networking - Custom Azure configuration file and updated deployment guide with instructions for using the custom deployment path
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/frontend/Dockerfile |
Multi-stage Dockerfile building Node.js frontend with Python runtime serving layer |
src/backend/Dockerfile |
Python-based backend Dockerfile with libicu-dev dependency for syntax checking |
infra/main_custom.bicep |
Comprehensive infrastructure-as-code template defining all Azure resources with configurable parameters for networking, monitoring, scaling, and redundancy |
docs/DeploymentGuide.md |
Added section documenting how to deploy local changes by renaming configuration files |
azure_custom.yaml |
Azure Developer CLI configuration specifying services, infrastructure path, and default parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/frontend/Dockerfile
Outdated
| @@ -0,0 +1,33 @@ | |||
| # Build stage | |||
| FROM node:18 AS build | |||
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base image node:18 includes the full Node.js environment which is unnecessary for the build stage. Consider using node:18-alpine to reduce image size and attack surface, which aligns with Docker best practices for multi-stage builds.
| FROM node:18 AS build | |
| FROM node:18-alpine AS build |
infra/main_custom.bicep
Outdated
| @description('Optional. Set the Image tag. Defaults to latest_2025-09-22_455.') | ||
| param imageVersion string = 'latest_2025-09-22_455' |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image version parameter contains a date '2025-09-22' which is in the future (September 2025), but the code review knowledge cutoff is January 2025. This appears to be a typo or placeholder value. Consider using a valid date or removing the date component from the default value.
| @description('Optional. Set the Image tag. Defaults to latest_2025-09-22_455.') | |
| param imageVersion string = 'latest_2025-09-22_455' | |
| @description('Optional. Set the Image tag. Defaults to latest.') | |
| param imageVersion string = 'latest' |
infra/main_custom.bicep
Outdated
| vmSize: vmSize ?? 'Standard_DS2_v2' | ||
| location: location | ||
| adminUsername: vmAdminUsername ?? 'JumpboxAdminUser' | ||
| adminPassword: vmAdminPassword ?? 'JumpboxAdminP@ssw0rd1234!' |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded default admin credentials are a security risk. While the password appears somewhat complex, having default credentials in code violates security best practices. Consider:
- Removing the default password entirely (force users to provide one)
- At minimum, add a warning comment that these defaults MUST be changed in production
- Better yet, generate a secure password dynamically if none is provided
| adminPassword: vmAdminPassword ?? 'JumpboxAdminP@ssw0rd1234!' | |
| // WARNING: If vmAdminPassword is not provided, a random password will be generated. You MUST change this in production. | |
| adminPassword: vmAdminPassword ?? newGuid() |
docs/DeploymentGuide.md
Outdated
| Go to `infra` directory | ||
| Rename `main.bicep` to `main_original.bicep` and `main_custom.bicep` to `main.bicep`. Continue with the [deploying steps](https://github.com/microsoft/Modernize-your-code-solution-accelerator/blob/ve-Local-readme/docs/DeploymentGuide.md#deploying-with-azd). |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GitHub URL contains a branch reference 've-Local-readme' which appears to be a feature branch rather than a stable reference like 'main' or a specific release tag. Documentation links should point to stable references to ensure they remain valid and accessible. Update to reference 'main' branch or use a permalink to a specific commit.
| Rename `main.bicep` to `main_original.bicep` and `main_custom.bicep` to `main.bicep`. Continue with the [deploying steps](https://github.com/microsoft/Modernize-your-code-solution-accelerator/blob/ve-Local-readme/docs/DeploymentGuide.md#deploying-with-azd). | |
| Rename `main.bicep` to `main_original.bicep` and `main_custom.bicep` to `main.bicep`. Continue with the [deploying steps](https://github.com/microsoft/Modernize-your-code-solution-accelerator/blob/main/docs/DeploymentGuide.md#deploying-with-azd). |
docs/DeploymentGuide.md
Outdated
| 6. You can now delete the resources by running `azd down`, when you have finished trying out the application. | ||
| ### Deploy Your local changes |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section header "Deploy Your local changes" has inconsistent capitalization. "Your" should be lowercase for proper title case. Change to "Deploy your local changes".
| ### Deploy Your local changes | |
| ### Deploy your local changes |
infra/main_custom.bicep
Outdated
| //param vmAdminUsername string = take(newGuid(), 20) | ||
| param vmAdminUsername string? | ||
|
|
||
| @description('Optional. Admin password for the Jumpbox Virtual Machine. Set to custom value if enablePrivateNetworking is true.') | ||
| @secure() | ||
| //param vmAdminPassword string = newGuid() |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code should be removed rather than left in the file. The comments on lines 73 and 78 show alternate approaches using newGuid() but are left in place. Either:
- Remove these comments if the approach was rejected
- Document why this approach wasn't used if it's important for future reference
Keeping commented-out code reduces code clarity and maintainability.
| //param vmAdminUsername string = take(newGuid(), 20) | |
| param vmAdminUsername string? | |
| @description('Optional. Admin password for the Jumpbox Virtual Machine. Set to custom value if enablePrivateNetworking is true.') | |
| @secure() | |
| //param vmAdminPassword string = newGuid() | |
| param vmAdminUsername string? | |
| @description('Optional. Admin password for the Jumpbox Virtual Machine. Set to custom value if enablePrivateNetworking is true.') | |
| @secure() |
infra/main_custom.bicep
Outdated
| name: 'AZURE_AI_AGENT_PROJECT_CONNECTION_STRING' // This was not really used in code. | ||
| value: aiServices.outputs.aiProjectInfo.apiEndpoint | ||
| } | ||
| { |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "This was not really used in code." on line 691 suggests this environment variable may be unnecessary. If the variable is truly unused:
- Remove it from the configuration
- If it might be needed in the future, add a clearer explanation
Including unused configuration increases maintenance burden and confusion.
| name: 'AZURE_AI_AGENT_PROJECT_CONNECTION_STRING' // This was not really used in code. | |
| value: aiServices.outputs.aiProjectInfo.apiEndpoint | |
| } | |
| { |
infra/main_custom.bicep
Outdated
| image: !empty(frontendImageName) ? frontendImageName : 'cmsacontainerreg.azurecr.io/cmsafrontend:${imageVersion}' | ||
| name: 'cmsafrontend' | ||
| resources: { | ||
| cpu: '1' |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CPU resource is specified as a number (1) for the backend container but as a string ('1') for the frontend container. While both may work, this inconsistency should be corrected for maintainability. Use consistent formatting across both container definitions - preferably the numeric format without quotes.
| cpu: '1' | |
| cpu: 1 |
…uvicorn for frontend serving
…ng Python/uvicorn for frontend serving
…gurations, remove redundant files, and clarify paths for improved clarity and maintainability.
…and ensure correct file references for backend and frontend services.
Purpose
This pull request introduces significant improvements to the deployment process by adding custom Dockerfiles for both the backend and frontend services, as well as a new Azure deployment configuration. It also updates the deployment documentation to guide users on deploying local changes using the new configuration files.
Deployment and configuration enhancements:
azure_custom.yamlconfiguration file to define custom deployment parameters, service locations, and infrastructure settings for Azure deployments.DeploymentGuide.md) with instructions for deploying local changes using the new custom configuration and Bicep files.Dockerization of services:
Dockerfilefor the backend service to build a Python 3.11 container, install dependencies, and run the backend using Uvicorn.Dockerfilefor the frontend service to build the frontend with Node.js, then serve it using a Python 3.11 container and Uvicorn, including installation of Python dependencies.Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify that the following are valid
Other Information