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

[9.0][ADD] server_environment_ir_config_parameter #617

Merged
merged 5 commits into from
Dec 22, 2016

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Nov 20, 2016

No description provided.

file has precedence.

The user cannot write, create, or delete System Parameters that are defined in the
configuration files.
Copy link
Contributor

@moylop260 moylop260 Nov 20, 2016

Choose a reason for hiding this comment

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

What about use this configuration file similar to data xml file at start server?
I mean, odoo.conf to odoo.xml format (using StringIO) with xml_id and noupdate="0" to support changes and loading from convert_xml_import

Copy link
Contributor

@moylop260 moylop260 Nov 20, 2016

Choose a reason for hiding this comment

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

Here a example using tools.convert_file

Here a example using directly tools.convert_xml_import

We can use the last one but but with StringIO like as:

def serv_conf2xml(conf):
    # .... code here
    pass

fp_xml = StringIO(serv_conf2xml(serv_config.get('ir.config_parameter')))
tools.convert_xml_import(fp_xml, ...)

Copy link
Member Author

Choose a reason for hiding this comment

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

@moylop260 I use the mechanism that exists in the server_environment module and is very powerful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both options use the mechanism that exists in the server_environment module and both are very powerful.

The disadvantage of a inherit @ormcache('uid', 'key') is that will be use 2 caches:

  1. ormcache for you inherited method
  2. ormcache for original one

But with the option of load parameters directly to database from configuration file you won't need add python logic because will use the same original powerful way and will work with new version of odoo (for 10.0 this section change).

By the way, your idea is very good (Thanks!)

Copy link
Contributor

Choose a reason for hiding this comment

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

@moylop260 IMO one advantage of the conf file is that it's easily understood by system administrators and less errors prone than xml for this kind of audience. I like the idea of being able to display the environment ribbon just by specifying a value in my configuration file.

Copy link
Contributor

@moylop260 moylop260 Nov 20, 2016

Choose a reason for hiding this comment

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

@lmignon I like the configuration file too
I'm not talking about use a xml like as file input I'll explain it better (I hope) in the main description. (see #617 (comment))

@sbidoul
Copy link
Member Author

sbidoul commented Nov 20, 2016

@moylop260 actually I think I don't really understand the alternative you propose. Do you want to add XML support to server_environment?

@moylop260
Copy link
Contributor

moylop260 commented Nov 20, 2016

@sbidoul
Ok, sorry for my bad explanation.
I'll try it again with a example.

Input

Configuration file with [ir.config_parameter] section like as:

[wkhtml2pdf]
lib_path = /myHome/lib/wkhtmltopdf-linux-i386-0-9-9

[ir.config_parameter]
ircp_from_config=config_value

Process

(see new methods section)

Output

Database updated, inserting records to ir_config_parameter table with new parameters at start odoo server.

New methods

def serv_conf_parameter2xml(conf_parameter)
    """:param conf_parameter: Server configuration of `ir.config_parameter`
    :returns: xml odoo data string like as the following example:
<?xml version='1.0' encoding='utf-8'?>
<openerp>
    <data noupdate="0">
        <record model="ir.config_parameter" id="record_id">
            <field name="key">{{ key_parameter }}</field>
            <field name="value">{{ value_parameter }}</field>
        </record>
        ...
        <!-- for this example will be: -->
        <record model="ir.config_parameter" id="ircp_from_config">
            <field name="key">ircp_from_config</field>
            <field name="value">config_value</field>
        </record>
        ...
        <record model="ir.config_parameter" id="record_id_N">
            <field name="key">{{ key_parameter_N }}</field>
            <field name="value">{{ value_parameter_N }}</field>
        </record>
    </data>
</openerp>
    """
    # ... Code using a template (maybe jinja) to auto-create this xml file based on configuration file.

def load_server_conf_parameter():
     # ...
     parameters_from_configuration_file = serv_conf['ir.config_parameter']
     xml_str = serv_conf_parameter2xml(parameters_from_configuration_file)
     fp_xml = StringIO.StringIO(xml_str)
     # This way update the database using xml_id engine to create.
     # This way will support updates using xml_id engine.
     # You won't need change or inherit the current python logic of `ir.config_parameter` model.
     # You won't need a duplicated validation because the database have a unique() constraint
     # You won't need problems managing `ormcached` for the inherited method and the original one
     # This way is compatible with all odoo versions
     tools.load_xml_import(fp_xml, ...)

@sbidoul
Copy link
Member Author

sbidoul commented Nov 20, 2016

@moylop260 thanks for the clarification, I understand better now.

I can see some advantages to loading the values in the database at server startup.

The problem is that we need to load them with the right record ids if the parameters are also provided by data files in modules (like web_environment_ribbon does for instance), otherwise there will be duplicate key errors when loading the modules. So we'd need additional information in the config file to provide the record external ids.

I'm also worried about the xml file not correctly enforcing the value from the config file, depending on the module loading order.

That's why I came up with this compromise of reading the config file in get_param.

In the meantime I pushed a workaround for the double ormcache issue you mentioned. I also changed the write/create behaviour to override with the config values instead of raising an error.

@sbidoul sbidoul force-pushed the 9.0-server_env_ir_c_p-sbi branch 6 times, most recently from c8b4f33 to bd0cb2c Compare November 20, 2016 23:30
@lmignon
Copy link
Contributor

lmignon commented Nov 21, 2016

@moylop260 Thank you for the clarification. For sure I misunderstood your comment.

Copy link
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.

Good idea! (Code review only)

@moylop260
Copy link
Contributor

I'm not sure... but I won't block this one...

Copy link
Contributor

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

I'm worried about a silently change of values without a warning.
It could be good add a flag to identify the parameters where is extracted from configuration file and set readonly the name and value field


def get_param(self, cr, uid, key, default=False, context=None):
value = super(IrConfigParameter, self).get_param(
cr, uid, key, default=None, context=context)
Copy link
Contributor

Choose a reason for hiding this comment

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

default=default

Copy link
Member Author

Choose a reason for hiding this comment

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

default=None is on purpose. The test for default value is at the end of the method.

@sbidoul
Copy link
Member Author

sbidoul commented Nov 21, 2016

@moylop260

It could be good add a flag to identify the parameters where is extracted from configuration file and set readonly the name and value field

That's a good idea. But I'm a bit short on time at the moment. Would it be ok for you if I add that in the roadmap?

@sbidoul
Copy link
Member Author

sbidoul commented Nov 22, 2016

Roadmap updated, waiting for #612 to check travis.

@sbidoul sbidoul force-pushed the 9.0-server_env_ir_c_p-sbi branch from b1050c7 to 23949dd Compare December 7, 2016 13:26
@sbidoul
Copy link
Member Author

sbidoul commented Dec 7, 2016

Green after rebase. Good to go.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 8, 2016

@rousseldenis

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Excellent! I had this in head since a while.

@guewen guewen merged commit 1aadfc7 into OCA:9.0 Dec 22, 2016
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (13.0)
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