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.

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

fs_storage: add possibility to define options values from env. #303

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

adrienpeiffer
Copy link
Contributor

No description provided.

fs_storage/models/fs_storage.py Outdated Show resolved Hide resolved
fs_storage/models/fs_storage.py Outdated Show resolved Hide resolved
fs_storage/models/fs_storage.py Outdated Show resolved Hide resolved
@sbidoul sbidoul requested a review from lmignon December 3, 2023 11:13
@sbidoul
Copy link
Member

sbidoul commented Dec 3, 2023

Looks good (code review). This should simplify configuration by letting us configure everything in a record with embedded environment variables such as, say, AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LG overall. I'll test later

fs_storage/models/fs_storage.py Show resolved Hide resolved
Comment on lines +323 to +325
_logger.warning(
f"Environment variable {env_variable_name} is not set for "
f"fs_storage {self.display_name}."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_logger.warning(
f"Environment variable {env_variable_name} is not set for "
f"fs_storage {self.display_name}."
_logger.warning(
"Environment variable %s is not set for "
"fs_storage %s", env_variable_name, self.display_name


eval_options_from_env = fields.Boolean(
string="Resolve env vars",
help="Resolve options values starting with $ from environment variables",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would provide an example here

Copy link
Sponsor Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you @adrienpeiffer
Can you complete the documentation in https://github.com/OCA/storage/blob/16.0/fs_storage/readme/USAGE.rst to describe this new option for configuring a storage backend?
Can you also add a file 303.feature into the fs_storage/readme/newsfragment directory to describe this new feature. This file will be use at merge to fill the history section into the readme file and help users to know what changes have been made between versions.

@adrienpeiffer adrienpeiffer force-pushed the env-var-fs-attachment-ape branch 2 times, most recently from 5c3f9c4 to f2fe690 Compare December 22, 2023 14:53
@adrienpeiffer
Copy link
Contributor Author

adrienpeiffer commented Dec 22, 2023

@lmignon Could be merged for Xmas ?

@sbidoul
Copy link
Member

sbidoul commented Dec 22, 2023

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-303-by-sbidoul-bump-minor, awaiting test results.

@sbidoul
Copy link
Member

sbidoul commented Dec 22, 2023

@adrienpeiffer can you handle the two comments from Simone?

@sbidoul
Copy link
Member

sbidoul commented Dec 22, 2023

I have cancelled the merge.

@OCA-git-bot OCA-git-bot merged commit 7776bef into OCA:16.0 Dec 22, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 359ac46. Thanks a lot for contributing to OCA. ❤️

@sbidoul sbidoul deleted the env-var-fs-attachment-ape branch December 22, 2023 15:36
@adrienpeiffer
Copy link
Contributor Author

@sbidoul Seems not cancelled. I'll handle that in an other PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants