-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Enable multiple secrets dirs #372
Changes from 22 commits
1ff5008
f23349f
bfc9d7a
58b032f
fa433b1
90f56bc
376737e
5c5e465
048ff00
d59fcc3
cff5aaa
7f8e0fc
6289ae7
037bf84
039d22f
5e85247
170c68d
09e8c05
74604ed
6e05557
a8e6b36
098eaaa
16d4606
3a4cd95
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 |
---|---|---|
|
@@ -573,7 +573,7 @@ class SecretsSettingsSource(PydanticBaseEnvSettingsSource): | |
def __init__( | ||
self, | ||
settings_cls: type[BaseSettings], | ||
secrets_dir: str | Path | None = None, | ||
secrets_dir: PathType | None = None, | ||
case_sensitive: bool | None = None, | ||
env_prefix: str | None = None, | ||
env_ignore_empty: bool | None = None, | ||
|
@@ -594,14 +594,22 @@ def __call__(self) -> dict[str, Any]: | |
if self.secrets_dir is None: | ||
return secrets | ||
|
||
self.secrets_path = Path(self.secrets_dir).expanduser() | ||
secrets_dirs = [self.secrets_dir] if isinstance(self.secrets_dir, (str, os.PathLike)) else self.secrets_dir | ||
secrets_paths = [Path(p).expanduser() for p in secrets_dirs] | ||
self.secrets_paths = [] | ||
|
||
if not self.secrets_path.exists(): | ||
warnings.warn(f'directory "{self.secrets_path}" does not exist') | ||
for path in secrets_paths: | ||
if not path.exists(): | ||
warnings.warn(f'directory "{path}" does not exist') | ||
else: | ||
self.secrets_paths.append(path) | ||
|
||
if not len(self.secrets_paths): | ||
return secrets | ||
|
||
if not self.secrets_path.is_dir(): | ||
raise SettingsError(f'secrets_dir must reference a directory, not a {path_type_label(self.secrets_path)}') | ||
for path in self.secrets_paths: | ||
if not path.is_dir(): | ||
raise SettingsError(f'secrets_dir must reference a directory, not a {path_type_label(path)}') | ||
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. There is a test for this error that tests single dir. please add a test for multiple directories as well. |
||
|
||
return super().__call__() | ||
|
||
|
@@ -639,18 +647,20 @@ def get_field_value(self, field: FieldInfo, field_name: str) -> tuple[Any, str, | |
""" | ||
|
||
for field_key, env_name, value_is_complex in self._extract_field_info(field, field_name): | ||
path = self.find_case_path(self.secrets_path, env_name, self.case_sensitive) | ||
if not path: | ||
# path does not exist, we currently don't return a warning for this | ||
continue | ||
# paths reversed to match the last-wins behaviour of `env_file` | ||
for secrets_path in reversed(self.secrets_paths): | ||
path = self.find_case_path(secrets_path, env_name, self.case_sensitive) | ||
if not path: | ||
# path does not exist, we currently don't return a warning for this | ||
continue | ||
|
||
if path.is_file(): | ||
return path.read_text().strip(), field_key, value_is_complex | ||
else: | ||
warnings.warn( | ||
f'attempted to load secret file "{path}" but found a {path_type_label(path)} instead.', | ||
stacklevel=4, | ||
) | ||
if path.is_file(): | ||
return path.read_text().strip(), field_key, value_is_complex | ||
else: | ||
warnings.warn( | ||
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. There is a test for this warning that tests single dir. please add a test for multiple directories as well. |
||
f'attempted to load secret file "{path}" but found a {path_type_label(path)} instead.', | ||
stacklevel=4, | ||
) | ||
|
||
return None, field_key, value_is_complex | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1416,6 +1416,33 @@ class Settings(BaseSettings): | |
assert Settings().model_dump() == {'foo': 'foo_secret_value_str'} | ||
|
||
|
||
def test_secrets_path_multiple(tmp_path): | ||
d1 = tmp_path / 'dir1' | ||
d2 = tmp_path / 'dir2' | ||
d1.mkdir() | ||
d2.mkdir() | ||
(d1 / 'foo1').write_text('foo1_dir1_secret_value_str') | ||
(d1 / 'foo2').write_text('foo2_dir1_secret_value_str') | ||
(d2 / 'foo2').write_text('foo2_dir2_secret_value_str') | ||
(d2 / 'foo3').write_text('foo3_dir2_secret_value_str') | ||
|
||
class Settings(BaseSettings): | ||
foo1: str | ||
foo2: str | ||
foo3: str | ||
|
||
assert Settings(_secrets_dir=(d1, d2)).model_dump() == { | ||
'foo1': 'foo1_dir1_secret_value_str', | ||
'foo2': 'foo2_dir2_secret_value_str', # dir2 takes priority | ||
'foo3': 'foo3_dir2_secret_value_str', | ||
} | ||
assert Settings(_secrets_dir=(d2, d1)).model_dump() == { | ||
'foo1': 'foo1_dir1_secret_value_str', | ||
'foo2': 'foo2_dir1_secret_value_str', # dir1 takes priority | ||
'foo3': 'foo3_dir2_secret_value_str', | ||
} | ||
|
||
|
||
def test_secrets_path_with_validation_alias(tmp_path): | ||
p = tmp_path / 'foo' | ||
p.write_text('{"bar": ["test"]}') | ||
|
@@ -1535,6 +1562,28 @@ class Settings(BaseSettings): | |
Settings() | ||
|
||
|
||
def test_secrets_invalid_secrets_dir_multiple_all(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: str | ||
|
||
(d1 := tmp_path / 'dir1').write_text('') | ||
(d2 := tmp_path / 'dir2').write_text('') | ||
|
||
with pytest.raises(SettingsError, match='secrets_dir must reference a directory, not a file'): | ||
Settings(_secrets_dir=[d1, d2]) | ||
|
||
|
||
def test_secrets_invalid_secrets_dir_multiple_one(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: str | ||
|
||
(d1 := tmp_path / 'dir1').mkdir() | ||
(d2 := tmp_path / 'dir2').write_text('') | ||
|
||
with pytest.raises(SettingsError, match='secrets_dir must reference a directory, not a file'): | ||
Settings(_secrets_dir=[d1, d2]) | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_missing_location(tmp_path): | ||
class Settings(BaseSettings): | ||
|
@@ -1544,6 +1593,37 @@ class Settings(BaseSettings): | |
Settings() | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_missing_location_multiple_all(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: Optional[str] = None | ||
|
||
with pytest.warns() as record: | ||
Settings(_secrets_dir=[tmp_path / 'dir1', tmp_path / 'dir2']) | ||
|
||
assert len(record) == 2 | ||
assert record[0].category is UserWarning and record[1].category is UserWarning | ||
assert str(record[0].message) == f'directory "{tmp_path}/dir1" does not exist' | ||
assert str(record[1].message) == f'directory "{tmp_path}/dir2" does not exist' | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_missing_location_multiple_one(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: Optional[str] = None | ||
|
||
(d1 := tmp_path / 'dir1').mkdir() | ||
(d1 / 'foo').write_text('secret_value') | ||
|
||
with pytest.warns() as record: | ||
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. can't you use 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. Fixed for two cases where a single warning is emitted. For other cases, we have multiple warnings, and need a |
||
conf = Settings(_secrets_dir=[d1, tmp_path / 'dir2']) | ||
|
||
assert conf.foo == 'secret_value' # value obtained from first directory | ||
assert len(record) == 1 | ||
assert record[0].category is UserWarning | ||
assert str(record[0].message) == f'directory "{tmp_path}/dir2" does not exist' | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_file_is_a_directory(tmp_path): | ||
p1 = tmp_path / 'foo' | ||
|
@@ -1558,6 +1638,45 @@ class Settings(BaseSettings): | |
Settings() | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_file_is_a_directory_multiple_all(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: Optional[str] = None | ||
|
||
(d1 := tmp_path / 'dir1').mkdir() | ||
(d2 := tmp_path / 'dir2').mkdir() | ||
(d1 / 'foo').mkdir() | ||
(d2 / 'foo').mkdir() | ||
|
||
with pytest.warns() as record: | ||
Settings(_secrets_dir=[d1, d2]) | ||
|
||
assert len(record) == 2 | ||
assert record[0].category is UserWarning and record[1].category is UserWarning | ||
# warnings are emitted in reverse order | ||
assert str(record[0].message) == f'attempted to load secret file "{d2}/foo" but found a directory instead.' | ||
assert str(record[1].message) == f'attempted to load secret file "{d1}/foo" but found a directory instead.' | ||
|
||
|
||
@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex') | ||
def test_secrets_file_is_a_directory_multiple_one(tmp_path): | ||
class Settings(BaseSettings): | ||
foo: Optional[str] = None | ||
|
||
(d1 := tmp_path / 'dir1').mkdir() | ||
(d2 := tmp_path / 'dir2').mkdir() | ||
(d1 / 'foo').write_text('secret_value') | ||
(d2 / 'foo').mkdir() | ||
|
||
with pytest.warns() as record: | ||
conf = Settings(_secrets_dir=[d1, d2]) | ||
|
||
assert conf.foo == 'secret_value' # value obtained from first directory | ||
assert len(record) == 1 | ||
assert record[0].category is UserWarning | ||
assert str(record[0].message) == f'attempted to load secret file "{d2}/foo" but found a directory instead.' | ||
|
||
|
||
def test_secrets_dotenv_precedence(tmp_path): | ||
s = tmp_path / 'foo' | ||
s.write_text('foo_secret_value_str') | ||
|
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.
There is a test for this warning that tests single dir. please add a test for multiple directories as well.