search

OCA / server-env

Tools to manage environment-dependent configuration
GNU Affero General Public License v3.0
57 stars 157 forks source link

Compatibility between server_environment_ir_config_parameter and server_environment_data_encryption #114

Closed florian-dacosta closed 2 years ago

florian-dacosta commented 2 years ago

Hello, The module server_environment_ir_config_parameter depends on server_environment but does not really use its framework. (it does not use server_env_mixin ). As a consequence, if I am not mistaken, we can't have a environment dependent value stored in database, like it is possible with server_environment and as another consequence, the module is not compatible with server_environment_data_encryption (which allows to have a encrypted environement dependent value stored in the database)

I would like to know if there is a good reason no to depend on server_env_mixin ? At first sight I have the feeling it would make the module simpler and compatible with server_environment_data_encryption, but maybe I miss an important point ? The only thing that would be lost is the is_environment flag. But it seems it is only usefull to warn the user that the displayed value could be different than the real one (which is in the odoo config file). And AFAIK that would not be the case if we inherit server_env_mixin because the field become computed/not stored.

If there is no special reason, I will be happy to propose a refactore on this module to depend on server_env_mixin, for version 16 at least. But I am not a user of this module (yet), I don't know it much and I may be missing an important point related to the special behaviors on ir.config.parameter.

Any idea @simahawk @sbidoul @legalsylvain ?

legalsylvain commented 2 years ago

Hi. I don't know. I made minor contribution on server_environment_ir_config_parameter, but that's all. I don't know the mixin you are talking about.I f you want i test some refactor in V16, please ping me.

sbidoul commented 2 years ago

I don't know either. When I wrote server_environment_ir_config_parameter, server_env_mixin did not exist. I've no idea if server_env_mixin would be applicable to ir.config_param. That sounds a little bit scary to be honest :)

florian-dacosta commented 2 years ago

I don't know either. When I wrote server_environment_ir_config_parameter, server_env_mixin did not exist

I was hoping that could be the reason !

That sounds a little bit scary to be honest

That's true, it is a bit scary! But I think it could go without any problem. I can do it and see how it goes on version 14, and if it goes well, maybe push the refactore on the v16 branch if it is fine with every body.

florian-dacosta commented 2 years ago

Here is a first shot : https://github.com/akretion/server-env/tree/14-refactore-server_environment_ir_config_parameter A migration script will be needed. (that would do the same as the post_init_hook I guess) I think it works fine, but it does change the format in the config file, and it does not make the module simpler. But at least, it make it consistent with other server_env modules and as a consequence it makes it compatible with server_environment_data_encryption. (which allows to store the env dependent value in the database, secret included)

@sebastienbeau

florian-dacosta commented 2 years ago

I close the isse since it could be discussed here instead https://github.com/OCA/server-env/pull/115