Chore: handle v3 commands in CLI #2547
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Soda Core v4 CLI to detect and block legacy Soda v3 commands early, exiting with a clear guidance message instead of falling through to argparse errors.
Changes:
- Added early interception of legacy v3 CLI commands (
scan,scan_status,ingest,test_connection,simulate_anomaly_detection) and exit withLOG_ERRORS. - Added a regression test asserting
soda scan ...exits with code3. - Updated a small test table purpose string used by contract verification log formatting tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| soda-core/src/soda_core/cli/cli.py | Adds handle_legacy_commands() and invokes it before argument parsing to block v3 commands. |
| soda-tests/tests/components/test_cli.py | Imports execute and adds a test for legacy scan exit behavior. |
| soda-tests/tests/components/test_contract_verification_logs.py | Tweaks test table purpose string used by log formatting test setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_cli_v3_scan(): | ||
| sys.argv = [ | ||
| "soda", | ||
| "scan", | ||
| "-d", | ||
| "ds", | ||
| "-c", | ||
| "sodacl_snowflake/configuration.yml", | ||
| "sodacl_pg/checks.yml", | ||
| ] | ||
|
|
||
| with pytest.raises(SystemExit) as e: | ||
| execute() | ||
|
|
||
| assert e.value.code == 3 |
There was a problem hiding this comment.
This change adds special handling for 5 legacy v3 commands, but the new test only covers scan. Consider parametrizing the test over all entries in legacy_cmds so accidental regressions (e.g., typo in one command name) are caught.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
soda-core/src/soda_core/cli/cli.py
Outdated
| def handle_legacy_commands(): | ||
| legacy_cmds = [ | ||
| "scan", | ||
| "scan_status", | ||
| "ingest", | ||
| "test_connection", | ||
| "simulate_anomaly_detection", | ||
| ] |
There was a problem hiding this comment.
For maintainability, legacy_cmds would be clearer as a module-level constant (and preferably a set/frozenset for membership checks). That also makes it easier to reuse the same source of truth from tests.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <[email protected]>
* Initial plan * Parametrize test to cover all 5 legacy v3 commands Co-authored-by: m1n0 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: m1n0 <[email protected]>
|
|
* Initial plan * Refactor: Move legacy_cmds to module-level constant as frozenset Co-authored-by: m1n0 <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: m1n0 <[email protected]> Co-authored-by: Milan Lukac <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
paulteehan
left a comment
paulteehan
left a comment
There was a problem hiding this comment.
Technically fine, might change the wording of the error message
- Soda v3 commands are not supported (Soda v4 was run)
- Please run Soda v3, or execute only Soda v4 commands.
- See https://docs.soda.io/soda-v4/reference/cli-reference for more information.
mivds
left a comment
mivds
left a comment
There was a problem hiding this comment.
Agree with Paul's comment. I think the wording Please use the v3 version of Soda Core is not correct. Soda library users may also run into this, and they would be directed to the wrong package
|
Handles any v3 CLI command (with arguments or without) with the following message
Covered commands:
scan
scan_status
ingest
test_connection
simulate_anomaly_detection
(+ unrelated tiny test fix)