-
-
Notifications
You must be signed in to change notification settings - Fork 104
Add NestedSecretsSettings source #690
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
base: main
Are you sure you want to change the base?
Add NestedSecretsSettings source #690
Conversation
@hramezani the PR is ready for review. |
secrets = reduce( | ||
lambda d1, d2: dict((*d1.items(), *d2.items())), | ||
(self.load_secrets(p) for p in self.secrets_paths), | ||
) |
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.
We can also support nested directories using the nested delimiter e.g. a file path like db/password
could be mapped to db__password
, which would write that into a nested model db
with field password
.
# Transform keys for nested delimiter support
if self.secrets_nested_delimiter and self.secrets_nested_delimiter != os.sep:
transformed_secrets = {}
for key, value in secrets.items():
new_key = key.replace(os.sep, self.secrets_nested_delimiter)
transformed_secrets[new_key] = value
secrets = transformed_secrets
I see that the docs state that this is already supported, so I'm not sure.
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.
Not sure what you mean here)
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.
In my implementation I added this, but don't really remember why 😅
Maybe it was to also support the nested delimiter within the secret name i.e. a secret file named db__password
.
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.
Oh, I see, you mean the case of both nested directory and file name prefix. This is already supported: https://github.com/makukha/pydantic-settings/blob/c39546cbe742855b7c96f2f9b5bcc23e64f7e8f1/tests/test_source_nested_secrets.py#L130
pyproject.toml
Outdated
] | ||
testing = [ | ||
"coverage[toml]", | ||
"dirlay>=0.4.0,<0.5.0", |
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.
Can we not use this package?
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.
Yes, I will update tests.
|
||
def __call__(self) -> dict[str, Any]: | ||
res = super().__call__() | ||
# breakpoint() # this is the most informative place to debug |
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 we need this comment here?
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.
Nope, I can remove the whole method
elif self.secrets_dir_missing == 'error': | ||
raise SettingsError(f'directory "{path}" does not exist') | ||
else: | ||
raise SettingsError(f'invalid secrets_dir_missing value: {self.secrets_dir_missing}') |
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.
This can be moved above when we initialize the settings. if the user provides a wrong value, we can raise error sooner
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.
This method is already called in constructor, not sure I get your idea)
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.
The method name is validate_secrets_path
, but it raises an error if the value provided for secrets_dir_missing
is not valid. I would suggest separating these two validations. we can move the secrets_dir_missing
value validation to line 54 when we collect value for self.secrets_dir_missing
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.
Ok, now I see
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.
updated
Merged code and docs from pydantic-file-secrets v0.4.4