Conversation
orangit-artturi-laitakari
requested review from
Ossmoskosmos and
sergey-orangit
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Dockerized SQL Server development database setup and migration scripts for the Caruna Sopimusrekisteri repo, along with supporting developer documentation.
Changes:
- Added Docker Compose configuration and start scripts to run SQL Server locally and handle port 1433 conflicts.
- Added database migration scripts + Python generators for schema, lookup data, and sample PDFs.
- Added/updated project documentation; removed template-era docs.
Reviewed changes
Copilot reviewed 18 out of 24 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/tietokannan_kehitysymparisto.md | New end-to-end documentation for local DB dev environment and migrations |
| docs/slack/slack-naming-convention.md | Removed old template documentation |
| docs/rider_ohjeistus.md | Added Rider usage guidance for this repo |
| docs/index.md | Removed old template documentation index |
| docs/caruna_sopimusrekisteri_ohjeistus.md | Added system setup notes (configs + dev-branch notes) |
| docs/business_logiikan_kirjoitus_dotnetCorelle.md | Added guidance for writing business logic in .NET Core during modernization |
| docker-compose.yml | Adds SQL Server 2022 local container definition |
| database/scripts/upload_pdfs.py | Generates SQL inserts for embedding PDF binaries into migrations |
| database/scripts/start.sh | Starts DB container and stops other containers bound to 1433 |
| database/scripts/start.ps1 | Windows equivalent of start script |
| database/scripts/generate_schema.py | Generates 000_schema.sql from running DB |
| database/scripts/generate_inserts.py | Generates 001_lookup_data.sql from running DB |
| database/scripts/README.md | Documentation for DB helper scripts |
| database/migrations/sample_pdfs/README.md | Instructions for exporting PDFs and generating SQL script |
| database/migrations/README.md | Documentation for migration scripts and workflow |
| database/migrations/001_lookup_data.sql | Generated lookup/reference data migration |
| database/README.md | Top-level database folder documentation and quick start |
| .gitignore | Ignores DB backup/data artifacts and .bak files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| image: mcr.microsoft.com/mssql/server:2022-latest | ||
| container_name: caruna-db | ||
| hostname: caruna-db | ||
| user: "0:0" # Run as root to avoid macOS Docker volume permission issues |
There was a problem hiding this comment.
This configuration (a) hard-codes an SA password in-repo, (b) runs SQL Server as root, and (c) publishes 1433 on all host interfaces. For safer defaults even in dev: bind to loopback only (e.g., 127.0.0.1:1433:1433), move SA_PASSWORD to a non-committed .env (or require overriding via environment), and avoid running as root by fixing volume permissions via a one-time init/chown or using named volumes only.
| # Hyväksy EULA | ||
| - ACCEPT_EULA=Y | ||
| # SA-salasana (VAIN KEHITYSYMPÄRISTÖÖN!) | ||
| - SA_PASSWORD=DevPassword123! |
There was a problem hiding this comment.
This configuration (a) hard-codes an SA password in-repo, (b) runs SQL Server as root, and (c) publishes 1433 on all host interfaces. For safer defaults even in dev: bind to loopback only (e.g., 127.0.0.1:1433:1433), move SA_PASSWORD to a non-committed .env (or require overriding via environment), and avoid running as root by fixing volume permissions via a one-time init/chown or using named volumes only.
| ports: | ||
| # Julkaise portti 1433 hostille | ||
| - "1433:1433" |
There was a problem hiding this comment.
This configuration (a) hard-codes an SA password in-repo, (b) runs SQL Server as root, and (c) publishes 1433 on all host interfaces. For safer defaults even in dev: bind to loopback only (e.g., 127.0.0.1:1433:1433), move SA_PASSWORD to a non-committed .env (or require overriding via environment), and avoid running as root by fixing volume permissions via a one-time init/chown or using named volumes only.
| Write-Host "" | ||
|
|
||
| # Odota että SQL Server vastaa | ||
| Write-Host "⏳ Waiting for SQL Server to be ready (max 60 seconds)..." -ForegroundColor Yellow |
There was a problem hiding this comment.
PowerShell start script uses /opt/mssql-tools/bin/sqlcmd and omits -C, while other scripts/docs use /opt/mssql-tools18/bin/sqlcmd with -C. With the SQL Server 2022 image, this mismatch can cause the readiness check to fail (missing path and/or TLS validation failure). Align the PowerShell script to the same sqlcmd path and flags as docker-compose/start.sh (tools18 + -C).
| Write-Host " Still waiting... ($attempt/$maxAttempts)" -ForegroundColor Gray | ||
| } | ||
|
|
||
| $result = docker exec caruna-db /opt/mssql-tools/bin/sqlcmd -S localhost -U SA -P "DevPassword123!" -Q "SELECT 1" 2>$null |
There was a problem hiding this comment.
PowerShell start script uses /opt/mssql-tools/bin/sqlcmd and omits -C, while other scripts/docs use /opt/mssql-tools18/bin/sqlcmd with -C. With the SQL Server 2022 image, this mismatch can cause the readiness check to fail (missing path and/or TLS validation failure). Align the PowerShell script to the same sqlcmd path and flags as docker-compose/start.sh (tools18 + -C).
| **ConnectionStrings (Rivi 8)**\ | ||
| Korvatkaa merkittyihin kohtiin palvelimen ja tietokannan tiedot. | ||
|
|
||
| ## slnSopimusarkistoSiirto { |
There was a problem hiding this comment.
There are Markdown formatting typos: the bold headers end with \\** / **\\ inconsistently, and one section header has an extra {. This renders oddly and looks accidental. Normalize the escaping (use either plain **...** or a single trailing \\ only when you intentionally need a hard line break) and remove the stray {.
|
|
||
| import subprocess | ||
| import sys | ||
| import re |
There was a problem hiding this comment.
re is imported but not used anywhere in this file. Removing it avoids dead imports and keeps the script minimal.
| import re |
| """ | ||
| Upload real PDF files to database migration script. | ||
|
|
||
| Reads 3 exported PDF files from database/migrations/sample_pdfs/ | ||
| and generates SQL INSERT statements with hex-encoded binary data. | ||
|
|
||
| Uses fake test IDs to avoid references to non-existent Sopimus records. | ||
| """ |
There was a problem hiding this comment.
The script and generated SQL explicitly encourage embedding real production PDFs into version control. That is a high risk for sensitive data leakage (PII/business-confidential content) and may violate compliance requirements. Prefer generating synthetic test PDFs (or sanitized/anonymized samples with documented approval), and adjust messaging/comments to avoid production-data guidance; if real exports are ever required, ensure they are excluded from git and handled via a secure process.
| print("=" * 60) | ||
|
|
||
| with open(output_file, 'w', encoding='utf-8') as f: | ||
| f.write("-- Sample PDF Data (Real PDFs from production database)\n") |
There was a problem hiding this comment.
The script and generated SQL explicitly encourage embedding real production PDFs into version control. That is a high risk for sensitive data leakage (PII/business-confidential content) and may violate compliance requirements. Prefer generating synthetic test PDFs (or sanitized/anonymized samples with documented approval), and adjust messaging/comments to avoid production-data guidance; if real exports are ever required, ensure they are excluded from git and handled via a secure process.
| f.write("SET NOCOUNT ON;\n") | ||
| f.write("GO\n\n") | ||
|
|
||
| f.write("PRINT 'Loading 3 sample PDF files (real production PDFs)...';\n") |
There was a problem hiding this comment.
The script and generated SQL explicitly encourage embedding real production PDFs into version control. That is a high risk for sensitive data leakage (PII/business-confidential content) and may violate compliance requirements. Prefer generating synthetic test PDFs (or sanitized/anonymized samples with documented approval), and adjust messaging/comments to avoid production-data guidance; if real exports are ever required, ensure they are excluded from git and handled via a secure process.
Tietokanta migraatiot