fix: return auth_type even if credentials are not in config - #47
Conversation
Sorry, something went wrong.
|
Just confirming: this change has already been applied to the main branch as of four months ago. We can certainly add the test but it's not clear to me why anything would be "fixed" by applying this patch. What have I missed? |
Sorry, something went wrong.
Change `dbsqlcli.main.apply_credentials_from_cfg` to return 4 values even if `not cfg.get("credentials")` is true.
This is so that the CLI can be used only with environment variables.
- Fixes <#46>
I just rebased this onto your main branch, but the PR still shows a diff for dbsqlcli/main.py. If it was applied, why is it still showing up in diff? And why, if I revert the changes this PR makes to dbsqlcli/main.py, does the test fail? 20230511T003419W19 iwana@teekai.zoic.eu.org:~/sw/d/github.com/aucampia/databricks-sql-cli
$ git log -2
commit bbc776aa534f2b7e6b4ad1d9da6953ef727bf31e (HEAD -> aucampia-patch-1, origin/aucampia-patch-1)
Author: Iwan Aucamp <aucampia@gmail.com>
Date: Tue Apr 18 22:21:07 2023 +0000
add a test
commit 370a88295ee40513d4d634b32069b56407a3ce1e
Author: Iwan Aucamp <aucampia@gmail.com>
Date: Tue Apr 18 01:56:22 2023 +0200
fix: return auth_type even if credentials are not in config
Change `dbsqlcli.main.apply_credentials_from_cfg` to return 4 values even if `not cfg.get("credentials")` is true.
This is so that the CLI can be used only with environment variables.
- Fixes <https://github.com/databricks/databricks-sql-cli/issues/46>
20230511T003423W19 iwana@teekai.zoic.eu.org:~/sw/d/github.com/aucampia/databricks-sql-cli
$ git diff
diff --git a/dbsqlcli/main.py b/dbsqlcli/main.py
index 41d5b4e..3672be7 100644
--- a/dbsqlcli/main.py
+++ b/dbsqlcli/main.py
@@ -64,7 +64,7 @@ def apply_credentials_from_cfg(hostname, http_path, access_token, auth_type, cfg
"""
if not cfg.get("credentials"):
- return hostname, http_path, access_token, auth_type
+ return hostname, http_path, access_token
hostname = hostname or cfg.get("credentials", {}).get("host_name")
http_path = http_path or cfg.get("credentials", {}).get("http_path")
20230511T003428W19 iwana@teekai.zoic.eu.org:~/sw/d/github.com/aucampia/databricks-sql-cli
$ poetry run pytest -- test/test_cli.py
============================================================================ test session starts ============================================================================
platform linux -- Python 3.9.16, pytest-7.2.0, pluggy-1.0.0
rootdir: /home/iwana/sw/d/github.com/aucampia/databricks-sql-cli
collected 4 items
test/test_cli.py ...F [100%]
================================================================================= FAILURES ==================================================================================
____________________________________________________________________ test_passthrough_with_empty_config _____________________________________________________________________
def test_passthrough_with_empty_config():
> host_name, http_path, access_token, auth_type = apply_credentials_from_cfg(
HOST_NAME, HTTP_PATH, ACCESS_TOKEN, AuthType.DATABRICKS_OAUTH.value, {}
)
E ValueError: not enough values to unpack (expected 4, got 3)
test/test_cli.py:65: ValueError
========================================================================== short test summary info ==========================================================================
FAILED test/test_cli.py::test_passthrough_with_empty_config - ValueError: not enough values to unpack (expected 4, got 3)
======================================================================== 1 failed, 3 passed in 0.24s ======================================================================== |
Sorry, something went wrong.
|
Here is the diff I see in the "Files Changed" tab for this PR:
Can you create a permalink to where this is already applied? |
Sorry, something went wrong.
susodapop
left a comment
There was a problem hiding this comment.
I apologise for the confusion on this. Looking through this with fresh eyes this morning I agree with your synopsis. This change LGTM.
Sorry, something went wrong.
Signed-off-by: Jesse Whitehouse <jesse.whitehouse@databricks.com>
|
This has been released in https://github.com/databricks/databricks-sql-cli/releases/tag/v0.3.2 |
Sorry, something went wrong.
Change dbsqlcli.main.apply_credentials_from_cfg to return 4 values even if not cfg.get("credentials") is true.
This is so that the CLI can be used only with environment variables.