Skip to content

Conversation

@m1so
Copy link
Contributor

@m1so m1so commented Dec 10, 2025

Summary by CodeRabbit

  • Tests
    • Enhanced validation of private key handling for Snowflake connections.
    • Added coverage for DER-encoded and encrypted key formats, including passphrase-protected keys.
    • Tests now verify proper decoding/conversion to the expected binary key form before use.
    • Improved generation and encoding steps in tests to exercise key loading and decryption paths.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 10, 2025

📝 Walkthrough

Walkthrough

Tests in tests/unit/test_sql_execution.py were changed to generate RSA private keys at runtime and encode them as base64 PEM PKCS#8. Test assertions now expect DER-encoded private key bytes (unencrypted PKCS#8) to be provided in Snowflake connect_args as private_key. An encrypted PEM path was added and tested by supplying a passphrase and verifying the key is loaded and converted to unencrypted DER before use. Tests import base64, secrets, and cryptography primitives for key generation and conversions.

Sequence Diagram(s)

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing static hardcoded private keys with dynamically generated ones in Snowflake connection tests.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d0797d9 and 23b5e2a.

📒 Files selected for processing (1)
  • tests/unit/test_sql_execution.py (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.cursorrules)

**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13

**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always use Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g., headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g., --port 8080)

Files:

  • tests/unit/test_sql_execution.py
**/test_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Name test files with test_*.py pattern

Files:

  • tests/unit/test_sql_execution.py
🪛 Ruff (0.14.8)
tests/unit/test_sql_execution.py

272-272: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


273-273: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


321-321: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


322-322: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Build and push artifacts for Python 3.11
  • GitHub Check: Build and push artifacts for Python 3.9
  • GitHub Check: Build and push artifacts for Python 3.12
  • GitHub Check: Build and push artifacts for Python 3.13
  • GitHub Check: Build and push artifacts for Python 3.10
  • GitHub Check: Test - Python 3.10
  • GitHub Check: Test - Python 3.11
  • GitHub Check: Test - Python 3.9
🔇 Additional comments (3)
tests/unit/test_sql_execution.py (3)

1-13: Imports for key generation and encoding look correct and necessary.

All newly added imports (base64, secrets, serialization, rsa) are used and appropriate for the new Snowflake key-handling tests.


237-281: Runtime-generated Snowflake private key test correctly asserts DER connect_args.

Nice switch to a runtime RSA key and base64‑encoded PEM while asserting that _query_data_source receives an unencrypted PKCS8 DER private_key via connect_args, with the original snowflake_private_key removed from params.


283-329: Encrypted Snowflake private key path is covered with secure randomness and correct DER expectation.

Using secrets.token_urlsafe for the passphrase plus BestAvailableEncryption matches good practice, and the test correctly verifies that the decrypted key is passed on as unencrypted PKCS8 DER in connect_args.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Dec 10, 2025

📦 Python package built successfully!

  • Version: 1.1.6.dev5+0cc941a
  • Wheel: deepnote_toolkit-1.1.6.dev5+0cc941a-py3-none-any.whl
  • Install:
    pip install "deepnote-toolkit @ https://deepnote-staging-runtime-artifactory.s3.amazonaws.com/deepnote-toolkit-packages/1.1.6.dev5%2B0cc941a/deepnote_toolkit-1.1.6.dev5%2B0cc941a-py3-none-any.whl"

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a251c6b and d0797d9.

📒 Files selected for processing (1)
  • tests/unit/test_sql_execution.py (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.cursorrules)

**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13

**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always use Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g., headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g., --port 8080)

Files:

  • tests/unit/test_sql_execution.py
**/test_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Name test files with test_*.py pattern

Files:

  • tests/unit/test_sql_execution.py
🪛 Ruff (0.14.8)
tests/unit/test_sql_execution.py

272-272: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


273-273: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


287-287: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


321-321: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)


322-322: Use a regular assert instead of unittest-style assertEqual

Replace assertEqual(...) with assert ...

(PT009)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Test - Python 3.10
  • GitHub Check: Test - Python 3.11
  • GitHub Check: Test - Python 3.9
  • GitHub Check: Build and push artifacts for Python 3.10
  • GitHub Check: Build and push artifacts for Python 3.9
  • GitHub Check: Build and push artifacts for Python 3.12
  • GitHub Check: Build and push artifacts for Python 3.11
  • GitHub Check: Build and push artifacts for Python 3.13
🔇 Additional comments (2)
tests/unit/test_sql_execution.py (2)

1-1: LGTM!

Import additions are appropriate for dynamic key generation.

Also applies to: 6-6, 12-13


238-280: LGTM!

Dynamic key generation correctly replaces hardcoded PEM. Test logic properly verifies PEM-to-DER conversion in Snowflake connect_args.

@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.04%. Comparing base (0674b44) to head (bc9260e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #42   +/-   ##
=======================================
  Coverage   73.04%   73.04%           
=======================================
  Files          93       93           
  Lines        5149     5149           
  Branches      754      754           
=======================================
  Hits         3761     3761           
  Misses       1144     1144           
  Partials      244      244           
Flag Coverage Δ
combined 73.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deepnote-bot
Copy link

deepnote-bot commented Dec 10, 2025

🚀 Review App Deployment Started

📝 Description 🌐 Link / Info
🌍 Review application ra-42
🔑 Sign-in URL Click to sign-in
📊 Application logs View logs
🔄 Actions Click to redeploy
🚀 ArgoCD deployment View deployment
Last deployed 2025-12-10 14:32:27 (UTC)
📜 Deployed commit 134b10979978e467ce05374149a9893233777458
🛠️ Toolkit version 0cc941a

@m1so m1so requested a review from FilipPyrek December 10, 2025 14:25
@m1so m1so marked this pull request as ready for review December 10, 2025 14:28
@m1so m1so requested a review from a team as a code owner December 10, 2025 14:28
@m1so m1so merged commit a8da274 into main Dec 10, 2025
31 checks passed
@m1so m1so deleted the mb/update-snowflake-private-key-tests branch December 10, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants