[IMP] core: ConfigCleaner part 3 Disruptive config #2648
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
This was referenced Jun 26, 2023
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
from
f9da118
to
a78b1f1
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
ed9085f
to
9197aca
Compare
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
2 times, most recently
from
f27a0e6
to
a5c468f
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
9197aca
to
b02dacf
Compare
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
2 times, most recently
from
297220d
to
e9cb9aa
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
2 times, most recently
from
dba41c7
to
8e6e725
Compare
Julien00859
commented
Jun 28, 2023
| 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.
maybe use a dedicated dict for the addons-path cc @VincentSchippefilt
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
2 times, most recently
from
655d088
to
0ea095c
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
8e6e725
to
aa061da
Compare
Julien00859
changed the title
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
from
0ea095c
to
e05dd5d
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
aa061da
to
cca4cb5
Compare
Julien00859
force-pushed
the
master-ccleaner2-glorified-optparse-juc
branch
from
e05dd5d
to
6b1d662
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
cca4cb5
to
0f252e5
Compare
Julien00859
commented
Jul 12, 2023
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.
it should be a list, comma separated string
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
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. |
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
d2b1252
to
4e84856
Compare
Julien00859
changed the base branch from
master-ccleaner2-glorified-optparse-juc
to
master-config-cleanup-juc
Julien00859
force-pushed
the
master-config-cleanup-juc
branch
from
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.
Julien00859
force-pushed
the
master-config-cleanup-juc
branch
from
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.
Julien00859
force-pushed
the
master-config-cleanup-juc
branch
from
8f1932f
to
ca38632
Compare
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
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.
Julien00859
force-pushed
the
master-ccleaner3-chainmap-juc
branch
from
616d442
to
4832df6
Compare
Julien00859
force-pushed
the
master-config-cleanup-juc
branch
4 times, most recently
from
2380c15
to
46b2231
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is part of a more global effort regarding a cleanup of the configuration management