[16.0] [IMP] fs_storage: Remove dependancy on server_environment

[etobella]

Server Environment should never be a dependancy on a base module.

So, I removed and added a glue module to handle the dependancy.

@lmignon @sebastienbeau as original authors of the module, I am sure that you have something to share.

[pedrobaeza]

I think separating this is important for being able to use the suite of modules without using server_environment system, which in my opinion is also mixing deployment with business logic.

[lmignon]

@etobella This change is unfortunately far more complex.. A First PR already exists here with some comments describing the impacts of such a change.

[lmignon]

Can you avoid to bump the version manually.

[etobella]

No, because I added a migration script

[lmignon]

To be complete and support all the installation mode, we should also modify some files into the setup directory....
in setup/fs_storage/setup.py

import setuptools


setuptools.setup(
    setup_requires=['setuptools-odoo'],
    odoo_addon=True,
    # The dependencies are not strictly required for the module 
    # to be installed, but they are required to ensure
    # compatibility with the initial implemenation of the module
    # where the server_environment was a direct dependency of
    # the module even if not used by the user.
    install_requires=['odoo-addon-fs-storage-environment'],
)

@sbidoul Am I right?

The same should apply for all the addons where the split is done. (but the requirements must be adapted 😏 )

Thank you for your work.

[lmignon]

To add an item into the history you must add a new file into the readme/newsfragments direcctory. For a features you just need to create a file named '627.feature.rst' (see https://towncrier.readthedocs.io/en/stable/tutorial.html#creating-news-fragments) with one paragraph. The history will be populated by the bot at merge time with the right version number and date.

[etobella]

I don't get the install_requires, actually, it was done on the migration script, that will work in all scenarios, not only when using pypi for installation

[sbidoul]

The install_requires is important so a user doing an upgrade of odoo-addon-fs_storage with pip or uv will receive the new optional odoo-addon-fs_storage_environment dependency automatically in its addons path. The migration script will then make sure the new dependency is actually installed in the Odoo database.

[etobella]

yes but that only makes sense when making the upgrade, not on other cases, for me it shouldn't be there...

[sbidoul]

yes but that only makes sense when making the upgrade, not on other cases, for me it shouldn't be there...

@etobella I don't understand. What upgrade? When doing a pip install of an addon, it makes sense that the glue modules are installed in the addons path together with it. That does not mean they will be installed in the database.

[etobella]

@sbidoul We are making this change in order to avoid the need of such a module. Then, why do we add the dependency if it is not needed anymore? it does only have sense on the migration, if someone starts to use the module and install it with pip, we server_environemnt and fs_storage_environment are added to their code base. That makes no sense IMO

[lmignon]

@etobella We need to add this pip dependency to ensure that we will not break existing installation if these addons are upgraded in an environment managed by pip. For sure this specific pip requirement will be removed in version 20 since the dependency on server_environment will not be present from the start for these addons. This change will simply ensure that the new glue add-ons are included in the Python path if they are required by the migration scripts. However, for any new user of the addons who uses pip, apart from installing Python packages that are unnecessary for their installation, this will have no effect.

[lmignon]

@etobella dixmit#1 😏

[sbidoul]

I have pre-registered the 4 new fs_*_environment modules on PyPI.

[lmignon]

@etobella My last remaining concern is the modification of the fs_storafe\readme\history.rst file. Could you take into account my comment here: #627 (comment)? It would make the forward ports easier.

[simahawk]

LGTM

CC @ivantodorovich @twalter-c2c

[etobella]

@lmignon done

[ivantodorovich]

Thanks for the work done!

It looks good to me. Just have one question about the migration scripts
Pre-approving, though

[ivantodorovich]

Is that really enough to get fs_attachment_environment installed during the fs_attachment upgrade?

I would've bet you need to call button_install or button_immediate_install on the ir.module.module instead.

@yankinmax, you worked on this too, do you remember what you had to do for the other modules?

[giarve]

Indeed migration did not run properly for me and erased the field values for the existing "fs.storage" records in the production environment.

Not sure if this was one of the reasons.

I was able to quickly to recover the config from a backup

[lmignon]

/ocabot merge nobump

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-627-by-lmignon-bump-nobump, awaiting test results.

[OCA-git-bot]

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