forked from datafold/data-diff
-
Notifications
You must be signed in to change notification settings - Fork 1
fix: remove hardcoded TrustServerCertificate and drop vertica from CI #15
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from data_diff.databases.base import ConnectError | ||
| from data_diff.databases.mssql import MsSQL | ||
|
|
||
|
|
||
| def _make_mssql(**extra_kw): | ||
| """Create an MsSQL instance with mocked threading and pyodbc.""" | ||
| with patch.object(MsSQL, "__attrs_post_init__", lambda self: None): | ||
| return MsSQL( | ||
| host="localhost", | ||
| port=1433, | ||
| user="sa", | ||
| password="secret", | ||
| database="testdb", | ||
| schema="dbo", | ||
| thread_count=1, | ||
| **extra_kw, | ||
| ) | ||
|
|
||
|
|
||
| class TestMsSQLConnectionArgs: | ||
| def test_trust_server_certificate_not_set_by_default(self): | ||
| db = _make_mssql() | ||
| assert "TrustServerCertificate" not in db._args | ||
|
|
||
| def test_user_supplied_trust_server_certificate_preserved(self): | ||
| db = _make_mssql(TrustServerCertificate="yes") | ||
| assert db._args["TrustServerCertificate"] == "yes" | ||
|
|
||
| def test_driver_is_set(self): | ||
| db = _make_mssql() | ||
| assert db._args["driver"] == "{ODBC Driver 18 for SQL Server}" | ||
|
|
||
| def test_none_values_filtered_from_args(self): | ||
| db = _make_mssql() | ||
| for v in db._args.values(): | ||
| assert v is not None | ||
|
|
||
|
|
||
| class TestMsSQLConnectionErrors: | ||
| def test_ssl_error_provides_actionable_message(self): | ||
| db = _make_mssql() | ||
| mock_mssql = MagicMock() | ||
| mock_mssql.Error = type("Error", (Exception,), {}) | ||
| mock_mssql.connect.side_effect = mock_mssql.Error( | ||
| "[SSL Provider] The certificate chain was issued by an untrusted authority" | ||
| ) | ||
|
|
||
| with patch("data_diff.databases.mssql.import_mssql", return_value=mock_mssql): | ||
| with pytest.raises(ConnectError, match="TrustServerCertificate"): | ||
| db.create_connection() | ||
|
|
||
| def test_certificate_only_keyword_triggers_actionable_message(self): | ||
| db = _make_mssql() | ||
| mock_mssql = MagicMock() | ||
| mock_mssql.Error = type("Error", (Exception,), {}) | ||
| mock_mssql.connect.side_effect = mock_mssql.Error( | ||
| "The remote certificate was rejected by the verification procedure" | ||
| ) | ||
|
|
||
| with patch("data_diff.databases.mssql.import_mssql", return_value=mock_mssql): | ||
| with pytest.raises(ConnectError, match="TrustServerCertificate"): | ||
| db.create_connection() | ||
|
|
||
| def test_non_ssl_error_passes_through(self): | ||
| db = _make_mssql() | ||
| mock_mssql = MagicMock() | ||
| mock_mssql.Error = type("Error", (Exception,), {}) | ||
| mock_mssql.connect.side_effect = mock_mssql.Error("Login failed for user") | ||
|
|
||
| with patch("data_diff.databases.mssql.import_mssql", return_value=mock_mssql): | ||
| with pytest.raises(ConnectError, match="Login failed"): | ||
| db.create_connection() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,3 @@ | ||
| #!/bin/bash | ||
|
|
||
| if [ -n "$DATADIFF_VERTICA_URI" ] | ||
| then | ||
| echo "Check Vertica DB running..." | ||
| while true | ||
| do | ||
| if docker logs dd-vertica | tail -n 100 | grep -q -i "vertica is now running" | ||
| then | ||
| echo "Vertica DB is ready"; | ||
| break; | ||
| else | ||
| echo "Waiting for Vertica DB starting..."; | ||
| sleep 10; | ||
| fi | ||
| done | ||
| fi | ||
| # Placeholder: add service readiness checks here as needed. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Both CI workflows still execute
tests/waiting_for_stack_up.shimmediately beforepytest(.github/workflows/ci.yml:50-52and.github/workflows/ci_full.yml:42-44), but this commit turns that script into a no-op, so tests start right afterdocker compose up -dwith no service readiness gate. The threaded DB connectors only attempt initial connection once and persist failures via_init_errorinThreadedDatabase.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 👍 / 👎.