-
Notifications
You must be signed in to change notification settings - Fork 910
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
Allow users to pass credentials through environment variables #2178
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
d0cbcb8
Add failing test
jmholzer c0d784f
Add in-place environment variable resolution
jmholzer 74da5fc
Add test to ensure oc.env resolver is cleared after reading credentials
jmholzer 3149eeb
Refactor docstring on _resolve_environment_variables
jmholzer d07cb3b
Move credentials.yml creation to fixture
jmholzer 26aa770
Modify _resolve_environment_variables to only clear the oc.env resolv…
jmholzer 238ce4d
Move environment variable resolution to load_and_merge_dir_config
jmholzer fc7f81e
Add read_environment_variables to load_and_merge_dir_config docstring
jmholzer 606d370
Merge branch 'main' into feat/allow-credentials-via-env-variables
jmholzer 6cf50d3
Merge branch 'main' into feat/allow-credentials-via-env-variables
jmholzer bc96421
Merge branch 'main' into feat/allow-credentials-via-env-variables
jmholzer b3bbddc
Merge branch 'main' into feat/allow-credentials-via-env-variables
jmholzer e8fac8e
Add test for env resolver not being used when config key is not 'cred…
jmholzer 5ab867c
Merge branch 'feat/allow-credentials-via-env-variables' of github.com…
jmholzer c3ca516
Lint
jmholzer ca83b47
Add release note
jmholzer 6e3c7c8
Refactor _resolve_environment_variables to remove unnecessary logic c…
jmholzer 800df92
Add test_env_resolver_is_registered_after_loading
jmholzer 7e6727f
Fix test_env_resolver_is_registered_after_loading
jmholzer 4060639
Merge branch 'main' into feat/allow-credentials-via-env-variables
jmholzer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,10 @@ | |
import logging | ||
from glob import iglob | ||
from pathlib import Path | ||
from typing import Any, Dict, Iterable, List, Set # noqa | ||
from typing import Any, Dict, Iterable, List, Optional, Set # noqa | ||
|
||
from omegaconf import OmegaConf | ||
from omegaconf.resolvers import oc | ||
from yaml.parser import ParserError | ||
from yaml.scanner import ScannerError | ||
|
||
|
@@ -143,15 +144,21 @@ def __getitem__(self, key) -> Dict[str, Any]: | |
) | ||
patterns = [*self.config_patterns[key]] | ||
|
||
read_environment_variables = key == "credentials" | ||
|
||
# Load base env config | ||
base_path = str(Path(self.conf_source) / self.base_env) | ||
base_config = self.load_and_merge_dir_config(base_path, patterns) | ||
base_config = self.load_and_merge_dir_config( | ||
base_path, patterns, read_environment_variables | ||
) | ||
config = base_config | ||
|
||
# Load chosen env config | ||
run_env = self.env or self.default_run_env | ||
env_path = str(Path(self.conf_source) / run_env) | ||
env_config = self.load_and_merge_dir_config(env_path, patterns) | ||
env_config = self.load_and_merge_dir_config( | ||
env_path, patterns, read_environment_variables | ||
) | ||
|
||
# Destructively merge the two env dirs. The chosen env will override base. | ||
common_keys = config.keys() & env_config.keys() | ||
|
@@ -178,13 +185,19 @@ def __repr__(self): # pragma: no cover | |
f"config_patterns={self.config_patterns})" | ||
) | ||
|
||
def load_and_merge_dir_config(self, conf_path: str, patterns: Iterable[str]): | ||
def load_and_merge_dir_config( | ||
self, | ||
conf_path: str, | ||
patterns: Iterable[str], | ||
read_environment_variables: Optional[bool] = False, | ||
) -> Dict[str, Any]: | ||
"""Recursively load and merge all configuration files in a directory using OmegaConf, | ||
which satisfy a given list of glob patterns from a specific path. | ||
|
||
Args: | ||
conf_path: Path to configuration directory. | ||
patterns: List of glob patterns to match the filenames against. | ||
read_environment_variables: Whether to resolve environment variables. | ||
|
||
Raises: | ||
MissingConfigException: If configuration path doesn't exist or isn't valid. | ||
|
@@ -216,6 +229,8 @@ def load_and_merge_dir_config(self, conf_path: str, patterns: Iterable[str]): | |
for config_filepath in config_files_filtered: | ||
try: | ||
config = OmegaConf.load(config_filepath) | ||
if read_environment_variables: | ||
self._resolve_environment_variables(config) | ||
config_per_file[config_filepath] = config | ||
except (ParserError, ScannerError) as exc: | ||
line = exc.problem_mark.line # type: ignore | ||
|
@@ -266,6 +281,22 @@ def _check_duplicates(seen_files_to_keys: Dict[Path, Set[Any]]): | |
dup_str = "\n".join(duplicates) | ||
raise ValueError(f"{dup_str}") | ||
|
||
@staticmethod | ||
def _resolve_environment_variables(config: Dict[str, Any]) -> None: | ||
"""Use the ``oc.env`` resolver to read environment variables and replace | ||
them in-place, clearing the resolver after the operation is complete if | ||
it was not registered beforehand. | ||
|
||
Arguments: | ||
config {Dict[str, Any]} -- The configuration dictionary to resolve. | ||
""" | ||
if not OmegaConf.has_resolver("oc.env"): | ||
OmegaConf.register_new_resolver("oc.env", oc.env) | ||
OmegaConf.resolve(config) | ||
OmegaConf.clear_resolver("oc.env") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would have been nice if |
||
else: | ||
OmegaConf.resolve(config) | ||
|
||
@staticmethod | ||
def _clear_omegaconf_resolvers(): | ||
"""Clear the built-in OmegaConf resolvers.""" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can drop the
oc.
or simply support bothenv
andoc.env
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean drop the "oc." in:
register_new_resolver
)or in:
omegaconf.resolvers.oc.env
(second argument toregister_new_resolver
)?I personally wouldn't drop the
oc.
from 1., since this is the name assigned to the resolver byOmegaConf
when it is instantiated. For 2. I would be ok with losing the namespace from the import:from omegaconf.resolvers.oc import env
.