-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor the configuration parsing #513
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,62 +1,131 @@ | ||
import os | ||
from collections.abc import Mapping | ||
from typing import TYPE_CHECKING, Optional | ||
from contextlib import contextmanager | ||
from typing import Optional, Union | ||
|
||
from tomlkit import load | ||
from tomlkit import TOMLDocument, dump, load | ||
|
||
if TYPE_CHECKING: | ||
from tomlkit import TOMLDocument | ||
from datachain.utils import DataChainDir, global_config_dir, system_config_dir | ||
|
||
|
||
def read_config(datachain_root: str) -> Optional["TOMLDocument"]: | ||
config_path = os.path.join(datachain_root, "config") | ||
try: | ||
with open(config_path, encoding="utf-8") as f: | ||
return load(f) | ||
except FileNotFoundError: | ||
return None | ||
class Config: | ||
SYSTEM_LEVELS = ("system", "global") | ||
LOCAL_LEVELS = ("local",) | ||
|
||
# In the order of precedence | ||
LEVELS = SYSTEM_LEVELS + LOCAL_LEVELS | ||
|
||
CONFIG = "config" | ||
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. Not sure if I understand the reason behind this attribute 👀 Can't find any usage of this. |
||
|
||
def __init__( | ||
self, | ||
level: Optional[str] = None, | ||
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. Just wonder, will it be better from typing prospective to have |
||
): | ||
self.level = level | ||
|
||
self.init() | ||
|
||
@classmethod | ||
def get_dir(cls, level: Optional[str]) -> str: | ||
if level == "system": | ||
return system_config_dir() | ||
if level == "global": | ||
return global_config_dir() | ||
|
||
return DataChainDir.find().root | ||
|
||
def init(self): | ||
d = DataChainDir(self.get_dir(self.level)) | ||
d.init() | ||
|
||
def load_one(self, level: Optional[str] = None) -> TOMLDocument: | ||
config_path = DataChainDir(self.get_dir(level)).config | ||
|
||
def get_remote_config( | ||
config: Optional["TOMLDocument"], remote: str = "" | ||
) -> Mapping[str, str]: | ||
if config is None: | ||
return {"type": "local"} | ||
if not remote: | ||
try: | ||
remote = config["core"]["default-remote"] # type: ignore[index,assignment] | ||
except KeyError: | ||
with open(config_path, encoding="utf-8") as f: | ||
return load(f) | ||
except FileNotFoundError: | ||
return TOMLDocument() | ||
|
||
def load_config_to_level(self) -> TOMLDocument: | ||
merged_conf = TOMLDocument() | ||
|
||
for merge_level in self.LEVELS: | ||
if merge_level == self.level: | ||
break | ||
config = self.load_one(merge_level) | ||
if config: | ||
merge(merged_conf, config) | ||
|
||
return merged_conf | ||
|
||
def read(self) -> TOMLDocument: | ||
if self.level is None: | ||
return self.load_config_to_level() | ||
return self.load_one(self.level) | ||
|
||
@contextmanager | ||
def edit(self): | ||
config = self.load_one(self.level) | ||
yield config | ||
|
||
self.write(config) | ||
|
||
def config_file(self): | ||
return DataChainDir(self.get_dir(self.level)).config | ||
|
||
def write(self, config: TOMLDocument): | ||
with open(self.config_file(), "w") as f: | ||
dump(config, f) | ||
|
||
def get_remote_config(self, remote: str = "") -> Mapping[str, str]: | ||
config = self.read() | ||
|
||
if not config: | ||
return {"type": "local"} | ||
try: | ||
remote_conf: Mapping[str, str] = config["remote"][remote] # type: ignore[assignment,index] | ||
except KeyError: | ||
raise Exception( | ||
f"missing config section for default remote: remote.{remote}" | ||
) from None | ||
except Exception as exc: | ||
raise Exception("invalid config") from exc | ||
|
||
if not isinstance(remote_conf, Mapping): | ||
raise TypeError(f"config section remote.{remote} must be a mapping") | ||
|
||
remote_type = remote_conf.get("type") | ||
if remote_type not in ("local", "http"): | ||
raise Exception( | ||
f'config section remote.{remote} must have "type" with one of: ' | ||
'"local", "http"' | ||
) | ||
|
||
if remote_type == "http": | ||
for key in ["url", "username", "token"]: | ||
if not remote: | ||
try: | ||
remote_conf[key] | ||
remote = config["core"]["default-remote"] # type: ignore[index,assignment] | ||
except KeyError: | ||
raise Exception( | ||
f"config section remote.{remote} of type {remote_type} " | ||
f"must contain key {key}" | ||
) from None | ||
elif remote_type != "local": | ||
raise Exception( | ||
f"config section remote.{remote} has invalid remote type {remote_type}" | ||
) | ||
return remote_conf | ||
return {"type": "local"} | ||
try: | ||
remote_conf: Mapping[str, str] = config["remote"][remote] # type: ignore[assignment,index] | ||
except KeyError: | ||
raise Exception( | ||
f"missing config section for default remote: remote.{remote}" | ||
) from None | ||
except Exception as exc: | ||
raise Exception("invalid config") from exc | ||
|
||
if not isinstance(remote_conf, Mapping): | ||
raise TypeError(f"config section remote.{remote} must be a mapping") | ||
|
||
remote_type = remote_conf.get("type") | ||
if remote_type not in ("local", "http"): | ||
raise Exception( | ||
f'config section remote.{remote} must have "type" with one of: ' | ||
'"local", "http"' | ||
) | ||
|
||
if remote_type == "http": | ||
for key in ["url", "username", "token"]: | ||
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. Just wonder, should we also have 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. This may be part of future changes when implementing some functionality with the token access. |
||
try: | ||
remote_conf[key] | ||
except KeyError: | ||
raise Exception( | ||
f"config section remote.{remote} of type {remote_type} " | ||
f"must contain key {key}" | ||
) from None | ||
Comment on lines
+113
to
+117
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. Just a comment: Studio URL may be optional 👀 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. I was not sure if we will continue using this TBH. Keeping this part as it is for now. |
||
elif remote_type != "local": | ||
raise Exception( | ||
f"config section remote.{remote} has invalid remote type {remote_type}" | ||
) | ||
return remote_conf | ||
|
||
|
||
def merge(into: Union[TOMLDocument, dict], update: Union[TOMLDocument, dict]): | ||
"""Merges second dict into first recursively""" | ||
for key, val in update.items(): | ||
if isinstance(into.get(key), dict) and isinstance(val, dict): | ||
merge(into[key], val) # type: ignore[arg-type] | ||
else: | ||
into[key] = val | ||
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.
Looks good to me, it would be great to add tests for this class.
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.
Added tests and fixes in #514