Skip to content

Chore!: Make default_connection optional #4522

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 2 commits into from
May 26, 2025
Merged

Conversation

VaggelisD
Copy link
Contributor

Up until now the config's default_connection was default initialized to DuckDB for less setup friction.

However, if the gateway connection was not configured properly, this could lead to us silently replacing it with DuckDB.

Our tests largely depended on that fact so as to reduce boilerplate code, and by making it optional there were many failures introduced. For that matter, I went ahead and solved it by monkey-patching Config.get_connection().

@VaggelisD VaggelisD force-pushed the vaggelisd/default_connection branch from 155b8a1 to 9e17431 Compare May 23, 2025 12:04
@benfdking benfdking requested a review from Copilot May 23, 2025 12:26
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Makes the default_connection field optional and enforces explicit configuration by raising ConfigError when no connection is provided, and updates tests to cover this behavior.

  • Change default_connection default to None and update get_connection to error when unset
  • Add new integration tests for missing connection configuration
  • Monkey-patch Config.get_connection in test setup for lax fallback

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
sqlmesh/core/config/root.py Made default_connection optional and added error on missing connection in get_connection
tests/init.py Introduced original_get_connection and a lax override for testing, patching Config.get_connection
tests/core/test_integration.py Added test_missing_connection_config covering new error paths and importing ConfigError
Comments suppressed due to low confidence (3)

tests/core/test_integration.py:6138

  • The code uses mock.patch.object but mock is not imported in this file; add from unittest import mock or import mock at the top to avoid a NameError.
with mock.patch.object(Config, "get_connection", Config.original_get_connection):

tests/init.py:20

  • Avoid using a bare except: which catches all exceptions; catch specific exceptions (e.g., except ConfigError:) to prevent masking unrelated errors.
    except:

tests/init.py:12

  • [nitpick] The comment mentions Config._get_connection but the override actually replaces Config.get_connection; update the comment to reflect the correct method name.
# Replace the implementation of Config._get_connection with a new one that always

@VaggelisD VaggelisD force-pushed the vaggelisd/default_connection branch 2 times, most recently from abe7d7d to f6d10c8 Compare May 23, 2025 14:31
@VaggelisD VaggelisD force-pushed the vaggelisd/default_connection branch from f6d10c8 to 7aedc4e Compare May 23, 2025 14:55
@VaggelisD VaggelisD changed the title Chore: Make default_connection optional Chore!: Make default_connection optional May 26, 2025
@VaggelisD VaggelisD force-pushed the vaggelisd/default_connection branch from f4a3cf0 to 2013286 Compare May 26, 2025 11:06
@VaggelisD VaggelisD merged commit d565154 into main May 26, 2025
23 checks passed
@VaggelisD VaggelisD deleted the vaggelisd/default_connection branch May 26, 2025 15:03
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.

3 participants