Skip to content
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

Update default config to work out-of-the-box with flytectl demo #1384

Merged
merged 13 commits into from
Jan 10, 2023
15 changes: 10 additions & 5 deletions flytekit/clis/sdk_in_container/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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(f"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
Expand Down
10 changes: 5 additions & 5 deletions flytekit/configuration/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/.
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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")
),
)

Expand Down
12 changes: 9 additions & 3 deletions flytekit/configuration/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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.
Expand All @@ -80,10 +85,11 @@ def read_from_file(
return None
try:
v = cfg.get(self)
if v:
if _exists(v):
wild-endeavor marked this conversation as resolved.
Show resolved Hide resolved
return transform(v) if transform else v
except Exception:
...

return None


Expand Down Expand Up @@ -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

Expand Down
2 changes: 2 additions & 0 deletions tests/flytekit/unit/cli/pyflyte/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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():
Expand Down
2 changes: 1 addition & 1 deletion tests/flytekit/unit/configuration/configs/good.config
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ assumable_iam_role=some_role


[platform]

url=fakeflyte.com
insecure=false

[madeup]

Expand Down
4 changes: 4 additions & 0 deletions tests/flytekit/unit/configuration/configs/nossl.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
admin:
endpoint: dns:///flyte.mycorp.io
authType: Pkce
insecure: false
28 changes: 27 additions & 1 deletion tests/flytekit/unit/configuration/test_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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
Expand Down Expand Up @@ -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
6 changes: 6 additions & 0 deletions tests/flytekit/unit/configuration/test_internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 9 additions & 0 deletions tests/flytekit/unit/configuration/test_yaml_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand All @@ -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
Expand All @@ -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
2 changes: 1 addition & 1 deletion tests/flytekit/unit/remote/test_remote.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down