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

[16.0][ADD] fs_storage: Storage Addons Refactoring RFC #252

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

lmignon
Copy link
Contributor

@lmignon lmignon commented Mar 28, 2023

A PR to track changes to the initial proposal of #251

In this PR you can find a document describing the proposal and the implementation of the first part trough the addon fs_storage.

The pr #260 implements the new addon fs_attachment
The pr #261 implements the new addon fs_file

@lmignon lmignon changed the title [16.0] Storage Addons Refactoring RFC [16.0][ADD] fs_storage: Storage Addons Refactoring RFC Apr 17, 2023
@florian-dacosta
Copy link
Contributor

@lmignon Is this ready to test/review ? (I am asking because of the draft state)

This replace storage_backend + storage_backend submodules, right ? But storage_backend is already merged.
The idea is to keep these conccurent modules for v16 and drop the storage_backend in v17 ?

@lmignon
Copy link
Contributor Author

lmignon commented May 23, 2023

@florian-dacosta This addon is still in draft since I'm waiting the feedback from C2C (camptocamp/odoo-cloud-platform#418) to relicence to LGPL.

I'll try to finalize #262 in the coming days but I'm busy with others priorities even if #262 is a priority. I've a more complete implementation for the fs_file addon on my machine with the new widget etc...

@florian-dacosta
Copy link
Contributor

@lmignon Thanks for the feedback.
But this PR (fs_storage) does not really depend on the C2C module, so this one should be ready, no ?

I'll try to use this fs_storage in a project using presently storage_backend (but not storage_file) and do a review here then.

@lmignon
Copy link
Contributor Author

lmignon commented May 23, 2023

@lmignon Thanks for the feedback. But this PR (fs_storage) does not really depend on the C2C module, so this one should be ready, no ?

@florian-dacosta You're right, I'll change the status. Before I'll update the development_status to alpha while others companion addons are not finalized (even if I'm pretty sure that the API should not change)

@lmignon lmignon marked this pull request as ready for review May 24, 2023 09:16
Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

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

LGTM
I did some tests with Odoo filestore, gtp, sftp and it went quite well.
Some minor remarks but nothing important, so I approve already.
I have tested with a real implementation (module attachment_synchronize)

Thanks for the work !

required=True,
help="Technical code used to identify the storage backend into the code."
"This code must be unique. This code is used for example to define the "
"storage backend to store the attachments vi the configuration parameter "
Copy link
Contributor

Choose a reason for hiding this comment

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

vi -> via

"installable": True,
"depends": ["base", "component", "server_environment"],
Copy link
Contributor

Choose a reason for hiding this comment

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

storage_backend did depend on server_environment, probably because you usually want to have this kind of configuration depending on the environment and also because you usually need to store some secrets, which is better not to leave in clear in the database.
Is this removal intentional, because we'd prefer to have a glue module for this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@florian-dacosta it's an open question... I tend to prefer to have glue addon than having too much dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, before, we'd have different fields for each protocol (sftp_login, ftp_login, etc). Including server-env was avoiding a lot of small server-env glue module.
Now, we would just need one, since every thing is in the json field and we don't have extra fields per protocol.
So, no problem for me to have a glue module.


class FSStorage(models.Model):
_name = "fs.storage"
_backend_name = "fs_storage"
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not see it used anymore, is it still usefull ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch


# the next fields are used to display documentation to help the user
# to configure the backend
options_protocol = fields.Selection(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why this field is usefull as we could use protocol to display the right options_properties.
Maybe I missed something, but it would avoid some code duplication between _get_options_protocol and _get_protocols and also make sure you display the description of the chosen protocol.
It is a bit strange to have to choose a protocol and then to have to choose again to see the right description in the available option tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@florian-dacosta With the lib you can nest the protocol. Therefore, the same field can be used to get access to the protocol documentation for the main protocol and when configuring nested protocols into the options field.

This addon define a new model 'fs.storage'  used to get access to a filesystem storage (ftp, sftp, s3, azure, ...) through an unified interface provided by the 'fsspec' python library (https://filesystem-spec.readthedocs.io/en/latest)
@lmignon
Copy link
Contributor Author

lmignon commented Aug 24, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-252-by-lmignon-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b328d6e into OCA:16.0 Aug 24, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

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

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.

4 participants