[IMP] [9.0] Allow configuring runner from config file - #213
Conversation
In addition to the ODOO_CONNECTOR_CHANNELS environment variables, we can
configure the runner channels in the odoo configuration file like this:
[options]
connector_channels = root:4
As such we can configure the job runner entirely with config files and
command line arguments.
We still need to load the `connector` module on the command line, as
there is no way yet pass the `load` parameter through the config file.
Ignore whitespace around values, and tolerate missing entries that would be caused by trailing commas or commented lines when the channel configuration is provided through the Odoo configuration file. Also, accept line breaks channel entry separators along with commas, which make the configuration file more readable.
Specifying how to use the configuration file as an alternative to the environment variable.
|
Any opinion on this? What is necessary to move this forward? |
lasley
left a comment
lasley
left a comment
There was a problem hiding this comment.
Minor comment, non-blocking. Thanks for the forward porting the improvement 👍
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # Unfortunately, it is not possible to extend the Odoo |
There was a problem hiding this comment.
Should this maybe be a DocBlock for _channels?
There was a problem hiding this comment.
Perhaps... The first part of this comment was moved (and refrased) from jobrunner/__init__.py, so it didn't occur to me to change it into a docstring.
If I have to shave this yak some more due to @sbidoul's comments before merging this branch I'll take the opportunity to move this comment into a _channels() docstring as well. 😏
| >>> ChannelManager.split_strip("foo: bar baz\\n: fred:", ":") | ||
| ['foo', 'bar baz', 'fred', ''] | ||
| """ | ||
| return [x.strip() for x in s.split(sep, maxsplit)] |
There was a problem hiding this comment.
This is more a candidate for a simple function rather than a class method.
There was a problem hiding this comment.
It was already like this on the 8.0-branch version of this change.
I could easily fix this here, but I'm afraid it could impact future changes done to multiple branches.
Should I:
- fix it just here (and on the future branch 10 version of this PR)?
- leave it as is?
- And perhaps in the future do another PR for fixing this in the three branches?
There was a problem hiding this comment.
@leorochael I approved the PR already. So you can leave as is.
sbidoul
left a comment
sbidoul
left a comment
There was a problem hiding this comment.
Minor remark, other wise looks very good to me (code review, no test). Thanks for this work and care to update the documentation.
5 participants
A forward port of #210.
It is slightly different from the 8.0 version since, AFAICS, the 9.0 connector always runs with a
root:1channel configuration if theconnectormodule is--loaded server wide on Odoo and the environment variable is missing or set to empty.Which means the only way to "disable" the
jobrunneris not to load theconnectormodule at all. Which is fine IMO.So I moved the default channel configuration logic inside the Channel runner itself.
This could be considered a slightly backward incompatible change:
ChannelRunnerwas not invoked by--loading theconnectormodule server wide, and someone decided to manually invoke theChannelRunnerwithout passing a channel configuration:root:1os.env['ODOO_CONNECTOR_CHANNELS'].With this PR, if the
ChannelRunneris invoked manually without passing a channel configuration, then instead of defaulting directly toroot:1, the environment variable and the configuration file will be consulted instead, defaulting toroot:1only if both are missing.