From 99bde2564cb7ea36e8dff5d5ffbe23931d6df024 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 28 Oct 2022 11:22:13 -0700 Subject: [PATCH 1/2] fix: no password prompt on db update --- .../cli/superset/sync/native/command.py | 8 ++- .../cli/superset/sync/native/command_test.py | 59 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/preset_cli/cli/superset/sync/native/command.py b/src/preset_cli/cli/superset/sync/native/command.py index 43eaced4..812e874d 100644 --- a/src/preset_cli/cli/superset/sync/native/command.py +++ b/src/preset_cli/cli/superset/sync/native/command.py @@ -149,6 +149,9 @@ def native( # pylint: disable=too-many-locals, too-many-arguments base_url = URL(external_url_prefix) if external_url_prefix else None + # collecting existing database UUIDs so we know if we're creating or updating + existing_databases = {str(uuid) for uuid in client.get_uuids("database").values()} + # env for Jinja2 templating env = dict(pair.split("=", 1) for pair in option if "=" in pair) # type: ignore env["instance"] = url @@ -179,7 +182,10 @@ def native( # pylint: disable=too-many-locals, too-many-arguments config["external_url"] = str( base_url / str(relative_path), ) - if relative_path.parts[0] == "databases": + if ( + relative_path.parts[0] == "databases" + and config["uuid"] not in existing_databases + ): prompt_for_passwords(relative_path, config) verify_db_connectivity(config) diff --git a/tests/cli/superset/sync/native/command_test.py b/tests/cli/superset/sync/native/command_test.py index 01195ec4..ef4920b1 100644 --- a/tests/cli/superset/sync/native/command_test.py +++ b/tests/cli/superset/sync/native/command_test.py @@ -155,6 +155,7 @@ def test_native(mocker: MockerFixture, fs: FakeFilesystem) -> None: "database_name": "GSheets", "sqlalchemy_uri": "gsheets://", "is_managed_externally": False, + "uuid": "uuid1", } dataset_config = {"table_name": "test", "is_managed_externally": False} fs.create_file( @@ -182,6 +183,7 @@ def test_native(mocker: MockerFixture, fs: FakeFilesystem) -> None: "preset_cli.cli.superset.sync.native.command.SupersetClient", ) client = SupersetClient() + client.get_uuids.return_value = {} import_resources = mocker.patch( "preset_cli.cli.superset.sync.native.command.import_resources", ) @@ -206,6 +208,55 @@ def test_native(mocker: MockerFixture, fs: FakeFilesystem) -> None: import_resources.assert_has_calls([mock.call(contents, client, False)]) +def test_native_password_prompt(mocker: MockerFixture, fs: FakeFilesystem) -> None: + """ + Test the ``native`` command with databases that have masked passwords. + """ + root = Path("/path/to/root") + fs.create_dir(root) + database_config = { + "database_name": "Postgres", + "sqlalchemy_uri": "postgresql://user:XXXXXXXXXX@host:5432/dbname", + "is_managed_externally": False, + "uuid": "uuid1", + } + fs.create_file( + root / "databases/gsheets.yaml", + contents=yaml.dump(database_config), + ) + + SupersetClient = mocker.patch( + "preset_cli.cli.superset.sync.native.command.SupersetClient", + ) + client = SupersetClient() + client.get_uuids.return_value = {} + mocker.patch("preset_cli.cli.superset.sync.native.command.import_resources") + mocker.patch("preset_cli.cli.superset.main.UsernamePasswordAuth") + prompt_for_passwords = mocker.patch( + "preset_cli.cli.superset.sync.native.command.prompt_for_passwords", + ) + + runner = CliRunner() + + result = runner.invoke( + superset_cli, + ["https://superset.example.org/", "sync", "native", str(root)], + catch_exceptions=False, + ) + assert result.exit_code == 0 + prompt_for_passwords.assert_called() + + prompt_for_passwords.reset_mock() + client.get_uuids.return_value = {"1": "uuid1"} + result = runner.invoke( + superset_cli, + ["https://superset.example.org/", "sync", "native", str(root)], + catch_exceptions=False, + ) + assert result.exit_code == 0 + prompt_for_passwords.assert_not_called() + + def test_native_load_env( mocker: MockerFixture, fs: FakeFilesystem, @@ -222,6 +273,7 @@ def test_native_load_env( "database_name": "Postgres", "sqlalchemy_uri": '{{ env["SQLALCHEMY_URI"] }}', "is_managed_externally": False, + "uuid": "uuid1", } fs.create_file( root / "databases/postgres.yaml", @@ -232,6 +284,7 @@ def test_native_load_env( "preset_cli.cli.superset.sync.native.command.SupersetClient", ) client = SupersetClient() + client.get_uuids.return_value = {} import_resources = mocker.patch( "preset_cli.cli.superset.sync.native.command.import_resources", ) @@ -256,6 +309,7 @@ def test_native_load_env( "database_name": "Postgres", "sqlalchemy_uri": "postgres://host1", "is_managed_externally": False, + "uuid": "uuid1", }, ), } @@ -273,6 +327,7 @@ def test_native_external_url(mocker: MockerFixture, fs: FakeFilesystem) -> None: "sqlalchemy_uri": "gsheets://", "external_url": "https://repo.example.com/databases/gsheets.yaml", "is_managed_externally": True, + "uuid": "uuid1", } dataset_config = { "table_name": "test", @@ -292,6 +347,7 @@ def test_native_external_url(mocker: MockerFixture, fs: FakeFilesystem) -> None: "preset_cli.cli.superset.sync.native.command.SupersetClient", ) client = SupersetClient() + client.get_uuids.return_value = {} import_resources = mocker.patch( "preset_cli.cli.superset.sync.native.command.import_resources", ) @@ -378,6 +434,7 @@ def test_template_in_environment(mocker: MockerFixture, fs: FakeFilesystem) -> N "database_name": "GSheets", "sqlalchemy_uri": "gsheets://", "test": "{{ filepath }}", + "uuid": "uuid1", } fs.create_file( root / "databases/gsheets.yaml", @@ -388,6 +445,7 @@ def test_template_in_environment(mocker: MockerFixture, fs: FakeFilesystem) -> N "preset_cli.cli.superset.sync.native.command.SupersetClient", ) client = SupersetClient() + client.get_uuids.return_value = {} import_resources = mocker.patch( "preset_cli.cli.superset.sync.native.command.import_resources", ) @@ -407,6 +465,7 @@ def test_template_in_environment(mocker: MockerFixture, fs: FakeFilesystem) -> N "is_managed_externally": False, "sqlalchemy_uri": "gsheets://", "test": "/path/to/root/databases/gsheets.yaml", + "uuid": "uuid1", }, ), } From e80460e2d2666311cd14fed265ed22fca76318e0 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Fri, 28 Oct 2022 11:55:33 -0700 Subject: [PATCH 2/2] Fix dataset bug --- .../cli/superset/sync/native/command.py | 5 ++ .../cli/superset/sync/native/command_test.py | 56 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/preset_cli/cli/superset/sync/native/command.py b/src/preset_cli/cli/superset/sync/native/command.py index 812e874d..eaa3eae8 100644 --- a/src/preset_cli/cli/superset/sync/native/command.py +++ b/src/preset_cli/cli/superset/sync/native/command.py @@ -4,6 +4,7 @@ import getpass import importlib.util +import json import logging import os from datetime import datetime, timezone @@ -188,6 +189,10 @@ def native( # pylint: disable=too-many-locals, too-many-arguments ): prompt_for_passwords(relative_path, config) verify_db_connectivity(config) + if relative_path.parts[0] == "datasets" and isinstance( + config.get("params"), str, + ): + config["params"] = json.loads(config["params"]) configs["bundle" / relative_path] = config diff --git a/tests/cli/superset/sync/native/command_test.py b/tests/cli/superset/sync/native/command_test.py index ef4920b1..20a9a523 100644 --- a/tests/cli/superset/sync/native/command_test.py +++ b/tests/cli/superset/sync/native/command_test.py @@ -208,6 +208,62 @@ def test_native(mocker: MockerFixture, fs: FakeFilesystem) -> None: import_resources.assert_has_calls([mock.call(contents, client, False)]) +def test_native_params_as_str(mocker: MockerFixture, fs: FakeFilesystem) -> None: + """ + Test the ``native`` command when dataset ``params`` are a string. + """ + root = Path("/path/to/root") + fs.create_dir(root) + database_config = { + "database_name": "GSheets", + "sqlalchemy_uri": "gsheets://", + "is_managed_externally": False, + "uuid": "uuid1", + } + dataset_config = { + "table_name": "test", + "is_managed_externally": False, + "params": '{"hello": "world"}', + } + fs.create_file( + root / "databases/gsheets.yaml", + contents=yaml.dump(database_config), + ) + fs.create_file( + root / "datasets/gsheets/test.yaml", + contents=yaml.dump(dataset_config), + ) + + SupersetClient = mocker.patch( + "preset_cli.cli.superset.sync.native.command.SupersetClient", + ) + client = SupersetClient() + client.get_uuids.return_value = {} + import_resources = mocker.patch( + "preset_cli.cli.superset.sync.native.command.import_resources", + ) + mocker.patch("preset_cli.cli.superset.main.UsernamePasswordAuth") + + runner = CliRunner() + result = runner.invoke( + superset_cli, + ["https://superset.example.org/", "sync", "native", str(root)], + catch_exceptions=False, + ) + assert result.exit_code == 0 + contents = { + "bundle/databases/gsheets.yaml": yaml.dump(database_config), + "bundle/datasets/gsheets/test.yaml": yaml.dump( + { + "table_name": "test", + "is_managed_externally": False, + "params": {"hello": "world"}, + }, + ), + } + import_resources.assert_has_calls([mock.call(contents, client, False)]) + + def test_native_password_prompt(mocker: MockerFixture, fs: FakeFilesystem) -> None: """ Test the ``native`` command with databases that have masked passwords.