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.

Sign up for GitHub

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

FLYTECTL_CONFIG env var should take highest precedence #1662

Merged
merged 3 commits into from
May 31, 2023
Merged

FLYTECTL_CONFIG env var should take highest precedence #1662

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f222cec
move env var up when looking for config file
wild-endeavor May 29, 2023
57da075
pr comment
wild-endeavor May 30, 2023
c8ec8bb
comment
wild-endeavor May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions flytekit/clis/sdk_in_container/pyflyte.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import typing

import grpc
Expand All @@ -17,6 +18,7 @@
from flytekit.clis.sdk_in_container.run import run
from flytekit.clis.sdk_in_container.serialize import serialize
from flytekit.clis.sdk_in_container.serve import serve
from flytekit.configuration.file import FLYTECTL_CONFIG_ENV_VAR
from flytekit.configuration.internal import LocalSDK
from flytekit.exceptions.base import FlyteException
from flytekit.exceptions.user import FlyteInvalidInputException
Expand Down Expand Up @@ -120,6 +122,14 @@ def main(ctx, pkgs: typing.List[str], config: str, verbose: bool):
if config:
ctx.obj[CTX_CONFIG_FILE] = config
cfg = configuration.ConfigFile(config)
# Set here so that if someone has Config.auto() in their user code, the config here will get used.
if FLYTECTL_CONFIG_ENV_VAR not in os.environ:
os.environ[FLYTECTL_CONFIG_ENV_VAR] = config
else:
cli_logger.warning(
f"Config file in arg {config} will be overridden by env var {FLYTECTL_CONFIG_ENV_VAR}:"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like args should take higher precedence than env. here is the doc for aws credentials precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure.

f" {os.environ[FLYTECTL_CONFIG_ENV_VAR]}"
)
if not pkgs:
pkgs = LocalSDK.WORKFLOW_PACKAGES.read(cfg)
if pkgs is None:
Expand Down
17 changes: 11 additions & 6 deletions flytekit/configuration/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,16 @@ def get_config_file(c: typing.Union[str, ConfigFile, None]) -> typing.Optional[C
Checks if the given argument is a file or a configFile and returns a loaded configFile else returns None
"""
if c is None:
# Env var always takes the highest precedence
flytectl_path_from_env = getenv(FLYTECTL_CONFIG_ENV_VAR, None)
if flytectl_path_from_env:
flytectl_path = Path(flytectl_path_from_env)
if flytectl_path.exists():
logger.info(f"Using flytectl/YAML config {flytectl_path.absolute()}")
return ConfigFile(str(flytectl_path.absolute()))
else:
logger.warning(f"flytectl config file {flytectl_path.absolute()} does not exist, ignoring...")

# See if there's a config file in the current directory where Python is being run from
current_location_config = Path("flytekit.config")
if current_location_config.exists():
Expand All @@ -251,13 +261,8 @@ def get_config_file(c: typing.Union[str, ConfigFile, None]) -> typing.Optional[C
logger.info(f"Using configuration from home directory {home_dir_config.absolute()}")
return ConfigFile(str(home_dir_config.absolute()))

# If not, see if the env var that flytectl sandbox tells the user to set is set,
# or see if there's something in the default home directory location
# If not see if there's something in the default home directory location
flytectl_path = Path(Path.home(), ".flyte", "config.yaml")
flytectl_path_from_env = getenv(FLYTECTL_CONFIG_ENV_VAR, None)

if flytectl_path_from_env:
flytectl_path = Path(flytectl_path_from_env)
if flytectl_path.exists():
logger.info(f"Using flytectl/YAML config {flytectl_path.absolute()}")
return ConfigFile(str(flytectl_path.absolute()))
Expand Down