From 581a5c66b6dec1d105f8f655918c405665ceebf6 Mon Sep 17 00:00:00 2001 From: Niels Bantilan Date: Tue, 10 Jan 2023 18:26:55 -0500 Subject: [PATCH] Update default config to work out-of-the-box with flytectl demo (#1384) Signed-off-by: Niels Bantilan --- flytekit/clis/sdk_in_container/helpers.py | 15 ++++++---- flytekit/configuration/__init__.py | 10 +++---- flytekit/configuration/file.py | 12 ++++++-- .../unit/cli/pyflyte/test_register.py | 2 ++ .../unit/configuration/configs/good.config | 2 +- .../unit/configuration/configs/nossl.yaml | 4 +++ .../flytekit/unit/configuration/test_file.py | 28 ++++++++++++++++++- .../unit/configuration/test_internal.py | 6 ++++ .../unit/configuration/test_yaml_file.py | 9 ++++++ tests/flytekit/unit/remote/test_remote.py | 2 +- 10 files changed, 74 insertions(+), 16 deletions(-) create mode 100644 tests/flytekit/unit/configuration/configs/nossl.yaml diff --git a/flytekit/clis/sdk_in_container/helpers.py b/flytekit/clis/sdk_in_container/helpers.py index 72246bcba4..6ac451be92 100644 --- a/flytekit/clis/sdk_in_container/helpers.py +++ b/flytekit/clis/sdk_in_container/helpers.py @@ -4,7 +4,7 @@ import click from flytekit.clis.sdk_in_container.constants import CTX_CONFIG_FILE -from flytekit.configuration import Config, ImageConfig +from flytekit.configuration import Config, ImageConfig, get_config_file from flytekit.loggers import cli_logger from flytekit.remote.remote import FlyteRemote @@ -25,10 +25,15 @@ def get_and_save_remote_with_click_context( :return: FlyteRemote instance """ cfg_file_location = ctx.obj.get(CTX_CONFIG_FILE) - cfg_obj = Config.auto(cfg_file_location) - cli_logger.info( - f"Creating remote with config {cfg_obj}" + (f" with file {cfg_file_location}" if cfg_file_location else "") - ) + cfg_file = get_config_file(cfg_file_location) + if cfg_file is None: + cfg_obj = Config.for_sandbox() + cli_logger.info("No config files found, creating remote with sandbox config") + else: + cfg_obj = Config.auto(cfg_file_location) + cli_logger.info( + f"Creating remote with config {cfg_obj}" + (f" with file {cfg_file_location}" if cfg_file_location else "") + ) r = FlyteRemote(cfg_obj, default_project=project, default_domain=domain) if save: ctx.obj[FLYTE_REMOTE_INSTANCE_KEY] = r diff --git a/flytekit/configuration/__init__.py b/flytekit/configuration/__init__.py index 5d65f8c3ca..220f9209ea 100644 --- a/flytekit/configuration/__init__.py +++ b/flytekit/configuration/__init__.py @@ -300,7 +300,7 @@ class PlatformConfig(object): :param endpoint: DNS for Flyte backend :param insecure: Whether or not to use SSL :param insecure_skip_verify: Wether to skip SSL certificate verification - :param console_endpoint: endpoint for console if differenet than Flyte backend + :param console_endpoint: endpoint for console if different than Flyte backend :param command: This command is executed to return a token using an external process. :param client_id: This is the public identifier for the app which handles authorization for a Flyte deployment. More details here: https://www.oauth.com/oauth2-servers/client-registration/client-id-secret/. @@ -311,7 +311,7 @@ class PlatformConfig(object): :param auth_mode: The OAuth mode to use. Defaults to pkce flow. """ - endpoint: str = "localhost:30081" + endpoint: str = "localhost:30080" insecure: bool = False insecure_skip_verify: bool = False console_endpoint: typing.Optional[str] = None @@ -529,7 +529,7 @@ def with_params( ) @classmethod - def auto(cls, config_file: typing.Union[str, ConfigFile] = None) -> Config: + def auto(cls, config_file: typing.Union[str, ConfigFile, None] = None) -> Config: """ Automatically constructs the Config Object. The order of precedence is as follows 1. first try to find any env vars that match the config vars specified in the FLYTE_CONFIG format. @@ -558,9 +558,9 @@ def for_sandbox(cls) -> Config: :return: Config """ return Config( - platform=PlatformConfig(endpoint="localhost:30081", auth_mode="Pkce", insecure=True), + platform=PlatformConfig(endpoint="localhost:30080", auth_mode="Pkce", insecure=True), data_config=DataConfig( - s3=S3Config(endpoint="http://localhost:30084", access_key_id="minio", secret_access_key="miniostorage") + s3=S3Config(endpoint="http://localhost:30002", access_key_id="minio", secret_access_key="miniostorage") ), ) diff --git a/flytekit/configuration/file.py b/flytekit/configuration/file.py index 793917cffe..23210e95f1 100644 --- a/flytekit/configuration/file.py +++ b/flytekit/configuration/file.py @@ -18,6 +18,11 @@ FLYTECTL_CONFIG_ENV_VAR = "FLYTECTL_CONFIG" +def _exists(val: typing.Any) -> bool: + """Check if a value is defined.""" + return isinstance(val, bool) or bool(val is not None and val) + + @dataclass class LegacyConfigEntry(object): """ @@ -63,7 +68,7 @@ def read_from_file( @dataclass class YamlConfigEntry(object): """ - Creates a record for the config entry. contains + Creates a record for the config entry. Args: switch: dot-delimited string that should match flytectl args. Leaving it as dot-delimited instead of a list of strings because it's easier to maintain alignment with flytectl. @@ -80,10 +85,11 @@ def read_from_file( return None try: v = cfg.get(self) - if v: + if _exists(v): return transform(v) if transform else v except Exception: ... + return None @@ -273,7 +279,7 @@ def set_if_exists(d: dict, k: str, v: typing.Any) -> dict: The input dictionary ``d`` will be mutated. """ - if v: + if _exists(v): d[k] = v return d diff --git a/tests/flytekit/unit/cli/pyflyte/test_register.py b/tests/flytekit/unit/cli/pyflyte/test_register.py index 4951d4be46..a6c0bb91d8 100644 --- a/tests/flytekit/unit/cli/pyflyte/test_register.py +++ b/tests/flytekit/unit/cli/pyflyte/test_register.py @@ -8,6 +8,7 @@ from flytekit.clients.friendly import SynchronousFlyteClient from flytekit.clis.sdk_in_container import pyflyte from flytekit.clis.sdk_in_container.helpers import get_and_save_remote_with_click_context +from flytekit.configuration import Config from flytekit.core import context_manager from flytekit.remote.remote import FlyteRemote @@ -34,6 +35,7 @@ def test_saving_remote(mock_remote): mock_context.obj = {} get_and_save_remote_with_click_context(mock_context, "p", "d") assert mock_context.obj["flyte_remote"] is not None + mock_remote.assert_called_once_with(Config.for_sandbox(), default_project="p", default_domain="d") def test_register_with_no_package_or_module_argument(): diff --git a/tests/flytekit/unit/configuration/configs/good.config b/tests/flytekit/unit/configuration/configs/good.config index 56bb837b00..06c2579d42 100644 --- a/tests/flytekit/unit/configuration/configs/good.config +++ b/tests/flytekit/unit/configuration/configs/good.config @@ -7,8 +7,8 @@ assumable_iam_role=some_role [platform] - url=fakeflyte.com +insecure=false [madeup] diff --git a/tests/flytekit/unit/configuration/configs/nossl.yaml b/tests/flytekit/unit/configuration/configs/nossl.yaml new file mode 100644 index 0000000000..f7acdde5a5 --- /dev/null +++ b/tests/flytekit/unit/configuration/configs/nossl.yaml @@ -0,0 +1,4 @@ +admin: + endpoint: dns:///flyte.mycorp.io + authType: Pkce + insecure: false diff --git a/tests/flytekit/unit/configuration/test_file.py b/tests/flytekit/unit/configuration/test_file.py index cb10bf42c0..3ce03f9c50 100644 --- a/tests/flytekit/unit/configuration/test_file.py +++ b/tests/flytekit/unit/configuration/test_file.py @@ -7,7 +7,8 @@ from pytimeparse.timeparse import timeparse from flytekit.configuration import ConfigEntry, get_config_file, set_if_exists -from flytekit.configuration.file import LegacyConfigEntry +from flytekit.configuration.file import LegacyConfigEntry, _exists +from flytekit.configuration.internal import Platform def test_set_if_exists(): @@ -21,6 +22,25 @@ def test_set_if_exists(): assert d["k"] == "x" +@pytest.mark.parametrize( + "data, expected", + [ + [1, True], + [1.0, True], + ["foo", True], + [True, True], + [False, True], + [[1], True], + [{"k": "v"}, True], + [None, False], + [[], False], + [{}, False], + ], +) +def test_exists(data, expected): + assert _exists(data) is expected + + def test_get_config_file(): c = get_config_file(None) assert c is None @@ -118,3 +138,9 @@ def test_env_var_bool_transformer(mock_file_read): # The last read should've triggered the file read since now the env var is no longer set. assert mock_file_read.call_count == 1 + + +def test_use_ssl(): + config_file = get_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), "configs/good.config")) + res = Platform.INSECURE.read(config_file) + assert res is False diff --git a/tests/flytekit/unit/configuration/test_internal.py b/tests/flytekit/unit/configuration/test_internal.py index 7f6be53a55..97e30b5612 100644 --- a/tests/flytekit/unit/configuration/test_internal.py +++ b/tests/flytekit/unit/configuration/test_internal.py @@ -77,3 +77,9 @@ def test_some_int(mocked): res = AWS.RETRIES.read(cfg) assert type(res) is int assert res == 5 + + +def test_default_platform_config_endpoint_insecure(): + platform_config = PlatformConfig() + assert platform_config.endpoint == "localhost:30080" + assert platform_config.insecure is False diff --git a/tests/flytekit/unit/configuration/test_yaml_file.py b/tests/flytekit/unit/configuration/test_yaml_file.py index 7e1c3eee98..ba2c61e158 100644 --- a/tests/flytekit/unit/configuration/test_yaml_file.py +++ b/tests/flytekit/unit/configuration/test_yaml_file.py @@ -14,6 +14,7 @@ def test_config_entry_file(): assert c.read() is None cfg = get_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), "configs/sample.yaml")) + assert cfg.yaml_config is not None assert c.read(cfg) == "flyte.mycorp.io" c = ConfigEntry(LegacyConfigEntry("platform", "url2", str)) # Does not exist @@ -26,6 +27,7 @@ def test_config_entry_file_normal(): cfg = get_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), "configs/no_images.yaml")) images_dict = Images.get_specified_images(cfg) assert images_dict == {} + assert cfg.yaml_config is not None @mock.patch("flytekit.configuration.file.getenv") @@ -43,6 +45,7 @@ def test_config_entry_file_2(mock_get): cfg = get_config_file(sample_yaml_file_name) assert c.read(cfg) == "flyte.mycorp.io" + assert cfg.yaml_config is not None c = ConfigEntry(LegacyConfigEntry("platform", "url2", str)) # Does not exist assert c.read(cfg) is None @@ -67,3 +70,9 @@ def test_real_config(): res = Credentials.SCOPES.read(config_file) assert res == ["all"] + + +def test_use_ssl(): + config_file = get_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), "configs/nossl.yaml")) + res = Platform.INSECURE.read(config_file) + assert res is False diff --git a/tests/flytekit/unit/remote/test_remote.py b/tests/flytekit/unit/remote/test_remote.py index 01688ea825..d7fde3215f 100644 --- a/tests/flytekit/unit/remote/test_remote.py +++ b/tests/flytekit/unit/remote/test_remote.py @@ -231,7 +231,7 @@ def test_generate_console_http_domain_sandbox_rewrite(mock_client): remote = FlyteRemote( config=Config.auto(config_file=temp_filename), default_project="project", default_domain="domain" ) - assert remote.generate_console_http_domain() == "http://localhost:30080" + assert remote.generate_console_http_domain() == "http://localhost:30081" with open(temp_filename, "w") as f: # This string is similar to the relevant configuration emitted by flytectl in the cases of both demo and sandbox.