-
Notifications
You must be signed in to change notification settings - Fork 147
Support multiple west config files per config level #867
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
base: main
Are you sure you want to change the base?
Support multiple west config files per config level #867
Conversation
|
Perfectly fine for me. I just wanted to show it to you for completeness. |
|
You could have waited for other opinions! They could still come but it's bit less likely in a closed PR. |
e33851f to
8f92d18
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #867 +/- ##
==========================================
+ Coverage 84.44% 84.55% +0.11%
==========================================
Files 11 11
Lines 3394 3432 +38
==========================================
+ Hits 2866 2902 +36
- Misses 528 530 +2
|
pdgendt
left a comment
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.
Honestly, I like the simplicity here. A user that sets the environment variable, to change a config file location, already needs to know what they are doing.
Allowing to pass in multiple files is in line with the ConfigParser's functionality.
8f92d18 to
3c9c122
Compare
pdgendt
left a comment
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.
@thorsten-klein could you split the --list-paths into it's own PR? Both this and the dropins PR have this addition, we could maybe move ahead with this simpler addition and clean up this and the dropins PR.
3c9c122 to
2fd4885
Compare
carlescufi
left a comment
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.
I am fine with this, thanks @thorsten-klein.
marc-hb
left a comment
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.
I found a couple issues while testing and reviewing but otherwise I agree this is definitely a minimal way to achieve the objective, nice!
src/west/configuration.py
Outdated
| if not self.paths: | ||
| raise WestNotFound('No config file exists that can be written') | ||
| if len(self.paths) > 1: | ||
| raise WestNotFound('Cannot write if multiple configs in use.') |
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.
| raise WestNotFound('Cannot write if multiple configs in use.') | |
| raise WestNotFound('Cannot write if multiple configs in use:' + os.pathsep.join(self.paths)) |
src/west/app/config.py
Outdated
| For each configuration level (local, global, and system) also multiple config | ||
| file locations can be specified. To do so, set according environment variable | ||
| to contain all paths (separated by 'os.pathsep', which is ';' on Windows or | ||
| ':' otherwise) |
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.
| ':' otherwise) | |
| ':' otherwise): Latter configuration files have precedence in such lists. |
src/west/configuration.py
Outdated
| self.paths = paths | ||
| read_files = self.cp.read(self.paths, encoding='utf-8') | ||
| if len(read_files) != len(self.paths): | ||
| raise FileNotFoundError(paths) |
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.
FileNotFoundError is not correct here. When a file in the list does not exist, things should still work and they apparently do (make sure you have a test for that, tests are all about edge cases).
On the other hand, while testing and trying to break this I found that a non-readable file throws this exception.
I also found that this WEST_CONFIG_LOCAL also throws this misleading exception: export WEST_CONFIG_LOCAL=~/.westconfig:$EMPTY_BECAUSE_TYPO
An empty path is apparently converted to "."? That seems wrong too.
Printing a stack trace is confusing here because it hides configparser which is at the root cause.
Finally, you should replace paths with os.pathsep.join(path) because this does not look great:
FileNotFoundError: [PosixPath('fu'), PosixPath('.'), PosixPath('bar')]
So I think you probably want to throw a WestNotFound (no stacktrace?) with a more generic message.
This is not cosmetic: users will misconfigure WEST_CONFIG_LOCAL at some point or another and west should give decent feedback when that happens.
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.
The list is normalized above, so that only paths remain in the list that really exist.
If the count of read files is not matching, something went wrong. I will adapt to raise some WestNotFound instead.
I will also extend the test to use a non-readable file and catch this error.
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.
WEST_CONFIG_LOCAL=~/.westconfig:$EMPTY_VAR_BECAUSE_TYPO. An empty path is apparently converted to "."? That seems wrong too.
The list is normalized above, so that only paths remain in the list that really exist.
I don't know where the "normalization" code was but it apparently didn't mind one of the paths to be the current directory "." (which always exists!)
I didn't test any newer version of this PR yet.
src/west/configuration.py
Outdated
| ret += self._system.paths | ||
| if self._local and location in [ConfigFile.LOCAL, ConfigFile.ALL]: | ||
| ret.append(self._local.path) | ||
| ret += self._local.paths |
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.
Is this related to this new feature? If not, separate commit please to reduce mental load. Reviewers don't want to guess.
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.
Yes, because now there is a list of paths stored (not a single path).
Therefore I cannot append, but I have to use extend or +=.
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.
For clarity it might be better to use extend here, not blocking though.
src/west/configuration.py
Outdated
|
|
||
| paths = self.get_paths(configfile) | ||
| if len(paths) > 1: | ||
| raise WestNotFound('Cannot write if multiple configs in use.') |
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.
Simple and smart! Nice.
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.
We have this same check at two places. Shall I keep both or only keep one? If yes, which one shall I keep?
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.
I prefer to have the check at the "lowest level", so keep the check in _write at least.
This can be used as a fast return path, or we let it fail later on, both are fine IMO.
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.
We need at both levels since otherwise _create is called with the path1;path2 which of course results in some errors.
We must catch it before we call this function.
|
EDIT: moved to new issue #871. I think #871 should be decided before this #867 PR gets merged. I just thought about another test case: relative paths. As it stands, this PR allows relative paths in Everyone please correct me but this looks like a really bad idea to me. I can easily imagine users wasting hours or even days because of subtle compilation differences depending on where they compiled the same thing. There are also serious security implications. So I don't think this should be allowed. This "bug" predates this PR. So it should probably be fixed before it. |
54cfdea to
032f4bb
Compare
marc-hb
left a comment
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.
(I would still like #871 to be discussed first and fixed separately)
src/west/configuration.py
Outdated
| # Normalize to a list of existing paths | ||
| paths = path | ||
| if isinstance(paths, Path): | ||
| paths = [Path(p) for p in str(paths).split(os.pathsep)] |
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.
This is confusing because it works only because a single Path is not always a real Path, sometimes it's a string with a list of paths shoehorned in a single, abused Path object! Which is why you convert here this fake Path into a string, split it and then convert back to a list of actual Path.
Rather than doing this, why not preserve correct typing and move the split to up in the callers? Right now the string -> Path conversion is performed at this point:
self._system_path = Path(_location(ConfigFile.SYSTEM))
self._global_path = Path(_location(ConfigFile.GLOBAL))
...After this PR, self._global_path can become something like this: PosixPath('/a/b/c:/d/e/f'). This is not a real Path and very misleading.
Instead you can probably do something like:
self._system_paths = [Path(p) for p in _locations(ConfigFile.SYSTEM)).split(os.pathsep)]
...(this could be a function like paths_to_str())
This would keep strings as strings, every Path as an actual, single Path and use a proper Path list elsewhere.
You can still perform the sanity checks in the common from_path()
Changing the name of these to a plural self._system_paths and switching to self._system_paths[0] when writing helps makes sure no place ever forgets to check that the length is 1.
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.
BTW it is this line that converts :$EMPTY_VAR: into the current directory.
src/west/configuration.py
Outdated
|
|
||
| def __init__(self, path: Path): | ||
| self.path = path | ||
| def from_path(path: Path | list[Path] | None) -> '_InternalCF | None': |
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.
This is never actually called with a list right now, is it?
I don't think any function or place in the code needs to accept alternatives like this, see below.
src/west/configuration.py
Outdated
|
|
||
| paths = self.get_paths(configfile) | ||
| if len(paths) > 1: | ||
| raise WestNotFound(f'Cannot write if multiple configs in use: {paths_to_str(paths)}') |
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.
Nit: I think the configfile == ConfigFile.ALL: check should be first because it's an internal bug.
BTW the elif on line 303 should be an if.
620351a to
33dee41
Compare
@thorsten-klein is this still in the works? For me it can go either way, as we've reduced the security concern raised in #871 in #871 (comment) This PR still only allows absolute paths, while @marc-hb is a proponent of having it relative to the workspace. I don't really have a strong opinion here. |
33dee41 to
d220ba7
Compare
src/west/configuration.py
Outdated
| if not paths: | ||
| return None | ||
| if not all(p.is_absolute() for p in paths): | ||
| raise WestNotFound(f"config file path(s) must be absolute: '{paths_to_str(paths)}'") |
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.
I don't think this is the right place to do this because _location() is used in a number of other places.
I'm working on fixing #871.
d220ba7 to
8178ece
Compare
7d86ab8 to
0756f8b
Compare
0756f8b to
bbc808f
Compare
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.
Thanks, I think we are getting close! I didn't review every line yet sorry but I think I already have enough comments to share for a first pass.
Note: all the tests must pass in every commit.
src/west/configuration.py
Outdated
| def __init__(self, path: Path): | ||
| self.path = path | ||
| def from_paths(paths: list[Path] | None) -> '_InternalCF | None': | ||
| paths = paths or [] |
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.
In which case does this line make a difference?
src/west/configuration.py
Outdated
|
|
||
| def _write(self): | ||
| if not self.paths: | ||
| raise WestNotFound('No config file exists that can be written') |
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.
I think this looks like an internal error, not something that could even happen.
| raise WestNotFound('No config file exists that can be written') | |
| assert(self.paths) |
|
|
||
| def _get(self, option, getter): | ||
| section, key = _InternalCF.parse_key(option) | ||
|
|
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.
Don't mix code changes with whitespace
https://wiki.openstack.org/wiki/GitCommitMessages#Things_to_avoid_when_creating_commits
| ret.extend(str_list_to_paths(paths)) | ||
| except WestNotFound: | ||
| pass | ||
|
|
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.
Distracting change, remove. This is a complex enough feature.
src/west/configuration.py
Outdated
| if not config_file: | ||
| raise ValueError(f"no config found for configfile '{configfile}'") | ||
| if len(config_file) > 1: | ||
| raise ValueError(f'Cannot write if multiple configs in use: {list_to_str(config_file)}') |
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.
Rather than converting back, simply save the _location() output for later.
|
|
||
| def list_paths(self, args): | ||
| config_paths = self.config.get_paths(args.configfile or ALL) | ||
| config_paths = self.config.get_paths(args.configfile or ALL, existing_only=True) |
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.
This looks like a fix for the previous commit?
Please correct me:
- before this PR,
--list-pathsshows only existing files. - after the previous commit,
--list-pathsshows some non-existing files. - thanks to this commit
--list-pathsgoes back to showing only existing files.
If correct then just squash with the previous commit and avoid the intermediate state.
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.
Missing space in 'tmp_west_topdir'function commit subject.
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.
This comment is still valid.
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.
ping
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.
This must be part of the main commit because all the tests must pass in every commit.
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.
I prefer adding new tests in a separate commit, after the commits with functional changes.
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.
Adding new tests in a later commit: no problem of course.
But not breaking existing tests and fixing them later, that's different.
For obvious git reasons, it must be possible to run and pass tests on every commit.
d2efd35 to
dca2152
Compare
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.
I prefer adding new tests in a separate commit, after the commits with functional changes.
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.
This comment is still valid.
For easier conversion between types, a configuration._Converter class is created with following static methods: - str_to_paths(paths, sep): helper function to split a 'paths' string at given 'sep' into a list of 'Path' elements - str_list_to_paths(paths): convert each list element from 'str' to 'Path'
9ee38dc to
d4691bd
Compare
src/west/configuration.py
Outdated
|
|
||
|
|
||
| def _gather_configs(cfg: ConfigFile, topdir: PathType | None) -> list[str]: | ||
| def _gather_configs(cfg: ConfigFile, topdir: PathType | None) -> list[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.
Not sure if we need to change the return type here? You can simply replace append with extend in this function and be done with it.
d4691bd to
71ba34d
Compare
multiple config files can be specified in the according environment variable for each config level (separated by 'os.pathsep', which is ';' on Windows or ':' otherwise). The config files are applied in the same order as they are specified, whereby values from later files override earlier ones. 'config --list-paths' prints all considered config files which are currently in use and exist. existing tests are adopted to work with the internal change of the return type of _location from 'str' to 'list[str]'
This helper function for tests is added. It temporarily creates a west topdir for the duration of the `with` block by creating a .west directory at given path. The diectory is removed again when the `with` block exits.
Verify that multiple config files can be specified in the according environment variable (WEST_CONFIG_SYSTEM, WEST_CONFIG_GLOBAL, WEST_CONFIG_LOCAL) for each config level.
71ba34d to
3089760
Compare
marc-hb
left a comment
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.
I think you are almost there.
| self.cp.write(f) | ||
|
|
||
|
|
||
| class _Converter: |
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.
I miss the value of wrapping two small static methods in this new class. Why not just functions? Or inside some existing class.
https://steve-yegge.blogspot.com/2006/03/execution-in-kingdom-of-nouns.html
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.
Sorry, but I don't see that they belong to class _InternalCF which says
class _InternalCF:
# For internal use only; convenience interface for reading and
# writing INI-style [section] key = value configuration files,
# but presenting a west-style section.key = value style API.
# Unlike the higher level and public "Configuration" class, there is
# one _InternalCF object for each location: LOCAL, GLOBAL,...
For understanding: Why are just functions better than functions within a (private) class?
| class _Converter: | ||
| @staticmethod | ||
| def str_to_paths(paths: str | None, sep: str = os.pathsep) -> list[Path]: | ||
| """Split a string into a list of Path objects using the given separator.""" |
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.
I'm concerned pydocs automatically make these part of the API, is that correct? They're too basic and unrelated to the API to be part of it.
|
|
||
| class _Converter: | ||
| @staticmethod | ||
| def str_to_paths(paths: str | None, sep: str = os.pathsep) -> list[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.
| def str_to_paths(paths: str | None, sep: str = os.pathsep) -> list[Path]: | |
| def parse_paths(paths: str | None, sep: str = os.pathsep) -> list[Path]: |
| self.paths = paths | ||
| read_files = self.cp.read(self.paths, encoding='utf-8') | ||
| if len(read_files) != len(self.paths): | ||
| raise WestNotFound(f"Error while reading one of '{paths}'") |
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.
| raise WestNotFound(f"Error while reading one of '{paths}'") | |
| raise MalformedConfig(f"Error while reading one of '{paths}'") |
All self.paths have been verified to .exists(), so "NotFound" is IMHO misleading.
|
|
||
| with open(self.path, 'w', encoding='utf-8') as f: | ||
| self.cp.write(f) | ||
| self._write() |
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.
Sorry but I don't like the fact that you "tentatively" change the value in memory and fail only at the last moment before actually writing. This means the _InternatCF state in memory is "impossible" for some duration. It's OK now but then it's reasonably likely to become a problem in the future whenever someone refactoring happens for some other feature or re-org.
I think you can avoid most of the duplication like this:
def set() / append() / etc.:
self.single_file_check() # raises exception
...
...
self._write()
def _write(self):
assert len(self.paths) == 1 # (in case some function forgot single_file_check)
with open(self.paths[0], 'w', encoding='utf-8') as f:
self.cp.write(f)
def single_file_check():
assert paths
len(self.paths) != 1
raise ValueError(f'Cannot write if multiple configs in use: {self.paths}')| write_config(config_l1, 'sec', 'l', '1', 'l1', '1') | ||
| write_config(config_l2, 'sec', 'l', '2', 'l2', '2') | ||
|
|
||
| # use a non-readable config file (does not work on Windows) |
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.
| # use a non-readable config file (does not work on Windows) | |
| # config file without read permission (does not work on Windows) |
| config_non_readable.touch() | ||
| config_non_readable.chmod(0o000) | ||
| os.environ["WEST_CONFIG_GLOBAL"] = f'{config_g1}{os.pathsep}{config_non_readable}' | ||
| _, stderr = cmd_raises('config --global some.section', WestNotFound) |
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.
"NotFound" is not right...
| config_g1 = config_dir / 'global 1' | ||
| config_g2 = config_dir / 'global 2' | ||
| config_l1 = config_dir / 'local 1' | ||
| config_l2 = config_dir / 'local 2' |
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.
Since you already imported Windows, try to break things using the "other" os.pathsep
if WINDOWS:
other_sep = ':'
else
other_sep = ';'
config_l2 = config_dir / 'local 2' + other_sep
| stdout = cmd('config --local sec.l1').rstrip() | ||
| assert stdout == '1' | ||
| stdout = cmd('config --local sec.l2').rstrip() | ||
| assert stdout == '2' |
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.
That's a lot of repetition. Use an array of key,value.
| msg = "'{file}' is relative but 'west topdir' is not defined" | ||
| cwd = pathlib.Path('any cwd') | ||
| cwd.mkdir() | ||
| with chdir(cwd): |
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.
I think you could have more coverage with less test code by:
- having some relative files in the test mix, always
- having some top-level test function that runs the ENTIRE test suite twice in two (or three) different directories. These tests do not involve git so I believe they are really quick to run.
marc-hb
left a comment
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.
- On one hand you want to keep filtering non-existing paths early, at
_InternalCF()construction time because it's very convenient. - On the other hand, now you need to remember non-existing files because you (rightfully) want to block modifications of
WEST_CONFIG_x=existing_fileA:non_existing_fileB.
So I think that's why you added an new existing_only boolean parameter to the get_paths() method but that confused me a lot because it has a default value. Also, depending on that parameter get_paths() now perform again the .exists() filtering that was already performed when constructing the _InternalCF() objects... a bit confusing.
I think it would be much clearer to have two different functions:
get_path_definitions()that includes non-existing filesget_existing_paths()that returns only existing files from already filtered_InternalCF()objects.
I bet they won't have any code in common.
Build/compute things only once, name them really well and then re-use them wherever they are needed.
BTW do you have a test that makes sure changes to WEST_CONFIG_x=existing_fileA:non_existing_fileB are blocked?
| if location in [ConfigFile.LOCAL, ConfigFile.ALL]: | ||
| ret.extend(self._local_paths) | ||
| if existing_only: | ||
| ret = [p for p in ret if p.exists()] |
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.
I don't see in which case this can make a difference. The self._*_paths have already been filtered in the _InternalCF.from_paths() (it's the reason why from_paths() exists). I understand why you want this but it does not seem to be happening. Do you have test coverage with missing files? Sorry I did not review the whole test code yet.
EDIT: this is probably one of the places where the existing/non-existing confusion hit me.
| if not configs: | ||
| raise WestNotFound(f'{configfile}: Cannot determine any config file') | ||
| if len(configs) > 1: | ||
| raise ValueError(f'Cannot set value if multiple configs in use: {configs}') |
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.
This exception is raised in multiple places: raise it from a single "check_single" very small function.
| all_config_files = [_location(cfg, topdir=topdir) for cfg in considered_locations] | ||
| for config_files in all_config_files: | ||
| if len(config_files) > 1: | ||
| raise ValueError(f'Error: Multiple config paths in use: {config_files}') |
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.
Is this too restrictive? Let's say I have two GLOBAL files and a single LOCAL file. Now this looks like this blocks me from deleting things in my single LOCAL file, doesn't it?
Maybe this can be done like this:
-
Look for the value in every "location" (GLOBAL,...) requested. Remember where it was found. Do NOT delete anything yet!
-
For each location where it was actually found in step 1, make sure there is single file in the definition of that location. Raise an exception if not. Do not delete anything yet.
-
Finally and atomically delete it from all the location where it was found (and requested).
-
Clearly explain this (or whatever better algorithm you find) in a comment :-)
-
Write some good tests for it.
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 restricts the configs only for considered_locations, which is [LOCAL] if --local was given. Nevertheless I have added a test for this to ensure that writing of a config is possible (even if the other "levels" use multiple configs)
| ''' | ||
| configs = self.get_paths(location) | ||
| if not configs: | ||
| raise WestNotFound(f'{configfile}: Cannot determine any config file') |
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.
You passed existing_only=False (the default), so I don't see how this could be raised. This looks like an assert, not an exception.
This is (another) alternative to PR #849. It is easier to discuss this solution while there is already some implementation existing.
Why?
In a big company there are often shared configs for build system and tooling (e.g., west and its configs).
Those generic configs must be applied first and contain some default or fallback values, if the user does not specify any value in any user config.
Depending on the company and team size, multiple different configs need to be stacked.
Remember, that users must always be able to overwrite values from those generic config files within their config.
Proposed Solution
Multiple config files can be specified in the according environment variable (
WEST_CONFIG_SYSTEM,WEST_CONFIG_GLOBAL,WEST_CONFIG_LOCAL) for each config level (separated byos.pathsep).The config files are applied in the same order as they are specified, whereby values from later files override earlier ones.
I personally like configuration through env variables, but I fear you don't.
Nevertheless I have prepared this proposal. Maybe it convinces you.
In a next step, this mechanism could also be combined with the approach from #849, so that multiple configs per config level are supported with a dropin config directory each. This would be the ultimative solution.