Connections Pane: AWS Redshift Support #10639
Conversation
|
E2E Tests 🚀 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds AWS Redshift support to the Connections Pane, allowing users to connect to and browse Redshift databases within Positron.
Key Changes:
- Implemented
RedshiftConnectionclass with support for listing databases, schemas, tables/views, and fields - Added comprehensive test coverage for Redshift connections following the established testing patterns
- Added
redshift_connectordependency to test requirements
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
extensions/positron-python/python_files/posit/positron/connections.py |
Adds RedshiftConnection class with methods for connection management, object listing, and data preview. Updates connection wrapping logic to recognize Redshift connections. |
extensions/positron-python/python_files/posit/positron/tests/test_connections.py |
Adds TestRedshiftConnectionsService class with comprehensive tests for connection registration, data containment checks, icon retrieval, object listing, field listing, and preview functionality. |
extensions/positron-python/python_files/posit/test-requirements.txt |
Adds redshift_connector as a test dependency. |
extensions/positron-python/python_files/posit/pinned-test-requirements.txt |
Pins redshift_connector to version 2.1.10 for Python versions < 3.14. |
| self.conn = conn | ||
|
|
||
| try: | ||
| # Unfortunatelly there's no public API to get the host, so we access the protected member. |
There was a problem hiding this comment.
Spelling error: "Unfortunatelly" should be "Unfortunately".
| # Unfortunatelly there's no public API to get the host, so we access the protected member. | |
| # Unfortunately there's no public API to get the host, so we access the protected member. |
| assert comm_id in connections_service.comms | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ("path_kind"), |
There was a problem hiding this comment.
The @pytest.mark.parametrize decorator has incorrect syntax. For a single parameter, use "path_kind" instead of ("path_kind"). The parentheses without a trailing comma create a string, not a tuple. While this may still work, it's inconsistent with the pattern used elsewhere (e.g., line 382 in the Databricks test) and could lead to confusion.
| ("path_kind"), | |
| "path_kind", |
| msg = _make_msg(params={"path": path}, method="contains_data", comm_id=comm_id) | ||
| dummy_comm.handle_msg(msg) | ||
| result = dummy_comm.messages[0]["data"]["result"] | ||
| assert result == (path_kind == "table") |
There was a problem hiding this comment.
[nitpick] Inconsistent boolean assertion. This test uses assert result == (path_kind == "table") while the equivalent Databricks test at line 397 uses assert result is (path_kind == "table"). For consistency with the existing codebase, consider using is instead of == when comparing boolean values.
| assert result == (path_kind == "table") | |
| assert result is (path_kind == "table") |
| "# Authentication steps may be incomplete, adjust as needed.\n" | ||
| "import redshift_connector\n" | ||
| "con = redshift_connector.connect(\n" | ||
| f" iam = True,\n" |
There was a problem hiding this comment.
Unnecessary f-string prefix. This line doesn't contain any interpolated values, so it should be a regular string: " iam = True,\n" instead of f" iam = True,\n". This is consistent with the pattern used in other connection implementations (e.g., DatabricksConnection at lines 1198-1199, 1202-1203).
| f" iam = True,\n" | |
| " iam = True,\n" |
| # Rollback on error to avoid transaction issues | ||
| # for subsequent queries | ||
| self.conn.rollback() | ||
| raise e |
There was a problem hiding this comment.
[nitpick] Use raise instead of raise e to re-raise the exception. This is more Pythonic and preserves the full traceback. The bare raise statement re-raises the current exception without modification.
| # Rollback on error to avoid transaction issues | ||
| # for subsequent queries | ||
| self.conn.rollback() | ||
| raise e |
There was a problem hiding this comment.
[nitpick] Use raise instead of raise e to re-raise the exception. This is more Pythonic and preserves the full traceback. The bare raise statement re-raises the current exception without modification.
Addresses #10432
Release Notes
New Features
Bug Fixes
QA Notes