-
Notifications
You must be signed in to change notification settings - Fork 1.2k
#181 Fix - Updated default data configuration to JSON #182
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
… Apps and File Share - Add warning in Step 6 SQLite migration section listing incompatible platforms (Container Apps with File Share, network file systems) - Add deployment-time alert in Docker section for Azure Container Apps users - Add comprehensive troubleshooting entry for 'database is locked' error with exact fix commands - Document root cause: SQLite file locking doesn't work over SMB/CIFS network protocols - Provide immediate solution: switch to FileSystem (JSON) storage which works perfectly with Azure File Share - List compatible vs incompatible platforms with visual markers - Prevents production deployment failures by warning users BEFORE they migrate to SQLite
- Add prominent warning in migration prompt modal for Azure Container Apps users - Alert users about SQLite incompatibility with Azure File Share (SMB/CIFS) - Prevent 'database is locked' errors by warning before migration - Recommend keeping JSON (FileSystem) storage for Container Apps with File Share
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 pull request changes the default storage provider from SQLite to JSON file-based storage (FileSystem) to improve compatibility with Azure Container Apps and other network file system deployments. The PR also upgrades the codebase to .NET 10.0 and comprehensively updates deployment documentation with clearer instructions, warnings about SQLite limitations, and revised Docker deployment procedures.
Key Changes:
- Changed default storage provider from SQLite to FileSystem in configuration files and startup logic
- Added prominent warnings about SQLite incompatibility with Azure Container Apps using File Share and other network file systems
- Updated Docker deployment instructions to require building images locally from source with corrected volume mount guidance
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/repository/appsettings.json | Changed default StorageProvider value from "SQLite" to "FileSystem" |
| src/Program.cs | Updated comments and fallback logic to reflect FileSystem as the default storage provider |
| docs/v5.0.0/wiki/V5.0.0_MIGRATION_GUIDE.md | Revised Docker deployment instructions with local build requirements, consistent port 8081 usage, and Docker volume configuration |
| docs/v5.0.0/V5.0.0_MIGRATION_GUIDE.md | Added critical warnings about SQLite/network file system incompatibility, updated Docker instructions (with port inconsistency issue), and expanded troubleshooting guidance |
| docs/v5.0.0/RELEASE_NOTES_v5.0.0.md | Clarified that SQLite is optional (not default) and added warnings about Azure Container Apps compatibility |
Comments suppressed due to low confidence (2)
docs/v5.0.0/V5.0.0_MIGRATION_GUIDE.md:373
- Docker Compose configuration is missing the top-level volumes declaration. When using a named volume like
azurenamingtoolvol, it should be declared in a top-levelvolumes:section. Add the following at the end of the docker-compose.yml:
volumes:
azurenamingtoolvol:While Docker Compose will create the volume automatically if it doesn't exist, explicitly declaring it is a best practice and makes the configuration clearer.
```yaml
version: '3.8'
services:
azurenamingtool:
build: . # Build from local Dockerfile
image: azurenamingtool:latest
container_name: azurenamingtool
ports:
- "8081:80" # Map host port 8081 to container port 80
volumes:
- azurenamingtoolvol:/app/settings # Use Docker volume for persistence
restart: unless-stopped
environment:
- ASPNETCORE_ENVIRONMENT=Production
**docs/v5.0.0/wiki/V5.0.0_MIGRATION_GUIDE.md:360**
* Docker Compose configuration is missing the top-level volumes declaration. When using a named volume like `azurenamingtoolvol`, it should be declared in a top-level `volumes:` section. Add the following at the end of the docker-compose.yml:
```yaml
volumes:
azurenamingtoolvol:
While Docker Compose will create the volume automatically if it doesn't exist, explicitly declaring it is a best practice and makes the configuration clearer.
**Docker Compose Example:**
```yaml
version: '3.8'
services:
azurenamingtool:
build: . # Build from local Dockerfile
image: azurenamingtool:latest
container_name: azurenamingtool
ports:
- "8081:80" # Map host port 8081 to container port 80
volumes:
- azurenamingtoolvol:/app/settings # Use Docker volume for persistence
restart: unless-stopped
environment:
- ASPNETCORE_ENVIRONMENT=Production
</details>
---
💡 <a href="/mspnp/AzureNamingTool/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
| ``` | ||
|
|
||
| 4. **Verify container started:** | ||
| 5. **Verify container started:** |
Copilot
AI
Dec 11, 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.
Numbering error in the deployment steps. The list jumps from step 3 to step 5, skipping step 4. This should be renumbered to step 4 instead of step 5 to maintain proper sequence.
| 5. **Verify container started:** | |
| 4. **Verify container started:** |
| 6. **VERIFY DEPLOYMENT:** | ||
| - Check container logs: `docker logs azurenamingtool | grep "Application started"` | ||
| - Access http://localhost and verify v5.0.0 loads | ||
| 7. **VERIFY DEPLOYMENT:** |
Copilot
AI
Dec 11, 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.
Numbering error in the deployment steps. This should be step 6 (not step 7) to maintain proper sequence after fixing the previous steps.
| 7. **VERIFY DEPLOYMENT:** | |
| 6. **VERIFY DEPLOYMENT:** |
| services: | ||
| azurenamingtool: | ||
| image: ghcr.io/mspnp/azurenamingtool:v5.0.0 | ||
| build: . # Build from local Dockerfile |
Copilot
AI
Dec 11, 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.
Docker Compose build context may be unclear. The step-by-step instructions tell users to cd src before running docker build, but the Docker Compose example uses build: . without specifying which directory to run docker-compose from. Since the Dockerfile is in the src/ directory, the docker-compose.yml should either be placed in the src/ directory, or the build context should be explicitly set to ./src in the compose file (e.g., build: ./src or build: { context: ./src }). Consider adding a note clarifying where to place or run the docker-compose.yml file.
| ``` | ||
|
|
||
| 5. Wait 30-60 seconds, then access the application at `http://localhost` | ||
| 6. Access the application at `http://localhost:8081` (or your chosen port) |
Copilot
AI
Dec 11, 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.
Numbering error in the deployment steps. This should be step 5 (not step 6) to maintain proper sequence after fixing step 4.
| services: | ||
| azurenamingtool: | ||
| image: ghcr.io/mspnp/azurenamingtool:v5.0.0 | ||
| build: . # Build from local Dockerfile |
Copilot
AI
Dec 11, 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.
Docker Compose build context may be unclear. The step-by-step instructions tell users to build from an extracted folder using docker build -t azurenamingtool . in the AzureNamingTool directory, but the Docker Compose example uses build: . without specifying which directory to run docker-compose from. If the Dockerfile is in the src/ subdirectory, users need to know whether to place the docker-compose.yml in the root directory and use build: ./src, or place it in the src directory. Consider adding clarification about the build context or file placement.
This pull request updates the Azure Naming Tool to default to JSON file-based storage instead of SQLite, improves documentation for deployment and migration (especially regarding Docker and Azure Container Apps), and upgrades the codebase to .NET 10.0. It adds strong warnings against using SQLite with network file systems due to file locking issues and revises guides to ensure users build Docker images locally and configure persistent storage correctly.
Storage Provider Defaults and Warnings
SQLitetoFileSysteminappsettings.json,Program.cs, and all fresh install logic; documentation now clearly states JSON file-based storage is the default for compatibility. [1] [2] [3]Docker and Deployment Instructions
.NET Framework Upgrade
Dockerfilefrom .NET 8.0 to .NET 10.0 to support the new release.Documentation Improvements
These changes ensure reliable deployments, especially in cloud/container scenarios, and help prevent common issues with SQLite on networked storage.