-
Notifications
You must be signed in to change notification settings - Fork 114
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
[IMP] core: ConfigCleaner part 3 Disruptive config #2648
base: master-config-cleanup-juc
Are you sure you want to change the base?
[IMP] core: ConfigCleaner part 3 Disruptive config #2648
Conversation
This PR targets the un-managed branch odoo-dev/odoo:master-ccleaner2-glorified-optparse-juc, it needs to be retargeted before it can be merged. |
f9da118
to
a78b1f1
Compare
ed9085f
to
9197aca
Compare
f27a0e6
to
a5c468f
Compare
9197aca
to
b02dacf
Compare
297220d
to
e9cb9aa
Compare
dba41c7
to
8e6e725
Compare
addons_path = self._cli_options.pop('addons_path', None) | ||
self._cli_options.clear() | ||
if addons_path is not None: | ||
self._cli_options['addons_path'] = addons_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use a dedicated dict for the addons-path cc @VincentSchippefilt
655d088
to
0ea095c
Compare
8e6e725
to
aa061da
Compare
0ea095c
to
e05dd5d
Compare
aa061da
to
cca4cb5
Compare
e05dd5d
to
6b1d662
Compare
cca4cb5
to
0f252e5
Compare
odoo/tools/config.py
Outdated
help='specify the SSL private key used for authentication') | ||
parser.add_option_group(group) | ||
|
||
# Database Group | ||
group = optparse.OptionGroup(parser, "Database related options") | ||
group.add_option("-d", "--database", dest="db_name", my_default=False, | ||
group.add_option("-d", "--database", dest="db_name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be a list, comma separated string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf odoo#128273
0f252e5
to
d2b1252
Compare
This PR targets the un-managed branch odoo-dev/odoo:master-ccleaner2-glorified-optparse-juc, it needs to be retargeted before it can be merged. |
d2b1252
to
4e84856
Compare
c5b8e4a
to
796aaa9
Compare
During the creation of the CLI and the parsing of the config file, the log level is not yet known and logging is not configured yet. This makes using both the logging and warnings libraries dangerous inside of this file as there is no garantee the message will be correctly shown.
Basically this revert 9dad29c which did re-introduce the option after someone from the community complained about it. Actually users can just set the `ODOO_RC` environment variable to achieve the same result.
Beside the standard `[options]` section, it is possible to define other "misc" sections in the configuration file and to access them via the `get_misc` method. To our knowledge, this API has never been used.
Various options such as `init`, `update`, `demo` and `load_language` are options that are modified at runtime. We believe it is a bad idea and we do not want to support new cases.
This commit is part of a larger refactor, see associated PR. Isolate building the CLI in a dedicated method so that it is possible to reference the `config` singleton instance when instanciating new options Isolate loading the default option values in a dedicated method so that it will be easier (later on) to write test cases. Rename (partially) `config.casts` to `config.options_index` so that it feels OK to use that datastructure even outside of cast consideration, the attribute will be fully renamed in a later commit.
This commit is part of a larger refactor, see associated PR. While `root_path` was defined as an option, in reality it cannot be set nor via the CLI, nor via the config file. Its value is fully defined at runtime and isn't modified anywhere.
796aaa9
to
8f1932f
Compare
This commit is part of a larger refactor, see associated PR. Use a MyOption object for all options, even the hidden ones in the command line interface. This makes it possible to have a fully defined option index this a single place to look for all meta information. The FileOnlyOption object makes it possible to completely hide some options from the CLI. Doing `--admin-passwd=x` produces an "no such option" error.
This commit is part of a larger refactor, see associated PR. The list wasn't maintained (see i18n options), using a dedicated attributes should help people define futur options.
This commit is part of a larger refactor, see associated PR. Many options are de-facto cli-only, some are explicitely not saved to or loaded from the config file, some can be loaded from the config file but are always erased at runtime, some are options to load/save the config file itself. Some i18n options *could* be loaded from the config file but this would be buggy as the various validations are only performed on the CLI.
This commit is part of a larger refactor, see associated PR. Using the new `cli_loadable` and `file_loadable` attributes, it is possible to list all keys that were defined in the hardcoded listing. The difference are `pg_path` and `test_enable` that were not present in the previous listing but present in the new comprehension: * for pg_path, it was set just bellow using the same cli->file fallback logic * for test_enable, its value is reset on `bool(test_tags)` anyway... The previous code was using two (almost) identical loops with two hardcoded listing. The two loops were at first difference but d5e7247 made them basically identical, almost 10 years ago.
8f1932f
to
ca38632
Compare
4e84856
to
616d442
Compare
This commit is part of a larger refactor, see associated PR. Store all option values defined at a same level in a dedicate read-only dictionnary, compose the various dictionnaries inside a chainmap for automatic failover. The order is as follow (most prioritary source first): 1. Runtime, options set via computation at runtime 2. CLI, options set via sys.argv 3. Configuration file, options found in -c/--config/.odoorc 4. Default, options hardcoded in the source code The 3 later sources: cli, config file and default, may still contain unparsed value, like comma-separated-list strings. It it the job of the runtime dictionnary to parse and re-expose those options inside its own dictionnary. This ease the cognitive complexity of `_parse_config` because it makes the order in which the various sources are loaded a bit less relevant. The CLI must still be loaded first for `--config` but the actual injection of values inside the `_file_config` and the `_cli_config` dictionnary is now irrevelant.
This commit is part of a larger refactor, see associated PR. Immediately cast all options as they are loaded from their source, don't leave dangling unparsed strings that need to be re-parsed inside of `_parse_args`. Use the opportunity to remove all bad `my_default=False` for non-boolean values. I really don't understand why people use `False` instead of `None` everywhere at Odoo. I get it for the xmlrpc API but every where else... (should had been in another commit) It was needed to add various parsers / formatters to optparse as it doesn't support simple `type=callable` like argparse (one of the MANY reasons why optparse is deprecated) Ensure addons_path and server_wide_module contains the default values.
This commit is part of a larger refactor, see associated PR. In the previous design, it was necessary to load options from the CLI after those from the config file as CLI options would replace the config file ones. At the same time, it was necessary to parse the CLI before the config file in order to find the -c/--config option. parse cli -> parse file -> load file options -> load cli options The parsing/loading of the CLI is interrupted by the parsing/loading of the configuration file. Thanks to the ChainMap, options can be loaded in any order, it is no more necessary to load the file options before the CLI ones. In this work we inverted the loading order, so cli before file. This allow for a simplification as the -c/--config is no longer special. The code related to the default rcfile path has been moved next to the other "smart" default options.
This commit is part of a larger refactor, see associated PR. Split `_parse_config` into `_load_cli_options`, `load` (which became `_load_file_options`) and `_postprocess_options`.
This commit is part of a larger refactor, see associated PR. The `odoo.conf` module was first introduced by Vo Minh Thu in 2011 with the following header message: > For now, configuration code is in openerp.tools.config. It is in mainly > unprocessed form, e.g. addons_path is a string with commas-separated > paths. The aim is to have code related to configuration (command line > parsing, configuration file loading and saving, ...) in this module > and provide real Python variables, e.g. addons_paths is really a list > of paths. The same year Vo Minh Thu resigned and nobody did maintain this module ever since. Fast forward 13 (!) years later, `odoo.tools.config` now expose processed options, i.e. addons_path is a list of paths.
The -d/--database option is used to configure the db_name option. At various places (essentially within the server subcommand) that option is used as a comma-separated list of database names and is iterated over. In other places (essentially within other subcommands) that option is used as a single database, a non-comma-separated string and is used as-is. This work cleans up the mess and makes it always a list. In places where a single value was expected, a warning is now emitted and the first item of the list is used.
616d442
to
4832df6
Compare
2380c15
to
46b2231
Compare
98f3009
to
987b9e3
Compare
fc72653
to
2a293ec
Compare
2a293ec
to
7750950
Compare
This PR is part of a more global effort regarding a cleanup of the configuration management