fix: remove hardcoded TrustServerCertificate and drop vertica from CI#15
fix: remove hardcoded TrustServerCertificate and drop vertica from CI#15
Conversation
Remove the debug `TrustServerCertificate=yes` override in the MSSQL connector that disabled TLS validation for all connections (MITM risk). Users can still opt in via URI params or TOML config. Drop vertica from CI docker compose since the `vertica/vertica-ce` image is no longer publicly pullable, which was failing all CI jobs. Vertica tests are already gated on DATADIFF_VERTICA_URI being set. Closes #2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1fcb4fbd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fi | ||
| done | ||
| fi | ||
| # Placeholder: add service readiness checks here as needed. |
There was a problem hiding this comment.
Both CI workflows still execute tests/waiting_for_stack_up.sh immediately before pytest (.github/workflows/ci.yml:50-52 and .github/workflows/ci_full.yml:42-44), but this commit turns that script into a no-op, so tests start right after docker compose up -d with no service readiness gate. The threaded DB connectors only attempt initial connection once and persist failures via _init_error in ThreadedDatabase.set_conn (data_diff/databases/base.py:1236-1243), so if MySQL/Postgres/ClickHouse are still booting, the suite can fail nondeterministically; keep a real wait condition (or equivalent compose wait behavior) before running tests.
Useful? React with 👍 / 👎.
…n CI Add a targeted error message when MSSQL connections fail due to SSL/certificate issues, guiding users to pass TrustServerCertificate explicitly. This helps users who previously relied on the removed hardcoded override. Use `docker compose up -d --wait` in CI workflows so services are healthy before pytest runs, replacing the now-empty wait script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…from CI Use case-insensitive matching for both "ssl" and "certificate" in the connection error detection, covering all ODBC driver message variants. Remove the now-empty waiting_for_stack_up.sh invocation from CI workflows since docker compose --wait already handles service readiness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
TrustServerCertificate=yesin the MSSQL connector that disabled TLS certificate validation for all connections (MITM risk). Users can still opt in via URI params or TOML config.verticafrom CI docker compose commands since thevertica/vertica-ceimage is no longer publicly pullable, which was causing all CI jobs to fail. Vertica tests are already gated onDATADIFF_VERTICA_URIbeing set.Closes #2
Test plan
TrustServerCertificateno longer appears inmssql.pypytest tests/test_mssql.py -v— 4 tests)🤖 Generated with Claude Code