Enforce top-level keys #1947
Enforce top-level keys #1947
Conversation
|
|
||
| for key in bad_keys: | ||
| warning = f"Unrecognized top-level key {key!r}" | ||
| log.warning(warning) |
There was a problem hiding this comment.
as mentioned elsewhere the double log / ctx feels a bit tedious, please refactor a bit in a future PR.
subiquity/server/server.py
Outdated
| self, | ||
| autoinstall_config: dict[str, Any], | ||
| ) -> dict[str, Any]: | ||
| """Removes all unkown keys from autoinstall config.""" |
There was a problem hiding this comment.
Comment is inverted, since we're returning autoinstall with known controllers removed, we're actually returning the unknowns
subiquity/server/server.py
Outdated
| server_config: dict[str, Any] = self._strip_controller_keys(autoinstall_config) | ||
| valid_keys: set[str] = set(self.base_schema["properties"].keys()) | ||
| top_level_keys: set[str] = set(server_config.keys()) | ||
| bad_keys: set[str] = top_level_keys - valid_keys |
There was a problem hiding this comment.
And because server_config is the unknowns, that means that this calculation of bad_keys works because it already has the unknowns stored in top_level_keys and we subtract any valid_keys (which won't be there) so we get the correct answer but in a bit of an obfuscated way.
subiquity/server/server.py
Outdated
| @@ -552,8 +553,93 @@ async def apply_autoinstall_config(self, context): | |||
| await controller.apply_autoinstall_config() | |||
| await controller.configured() | |||
|
|
|||
| def _strip_controller_keys( | |||
There was a problem hiding this comment.
I kind of want you to change how this function works, to have it return two dicts.
One dict of the known-good keys, one of the known-bad keys.
I think it would simplify the later operations, as
- you won't have to separately calculate bad_keys (this will tell you)
- the calculation of the good keys will be in the same spot (right now there are a few similar-looking things and the logic is spread over this function and the caller), and the calling function can just take action
subiquity/server/server.py
Outdated
|
|
||
| """ | ||
|
|
||
| server_config: dict[str, Any] = self._strip_controller_keys(autoinstall_config) |
There was a problem hiding this comment.
server_config is a bit generic, following the restructure of _strip_controller_keys maybe it could be good_keys / bad_keys?
| self.assertEqual(self.server.autoinstall_config, stripped_ai_data) | ||
|
|
||
| # Not OK in Version >= 2 | ||
| log_mock.reset_mock() |
There was a problem hiding this comment.
I guess I kind of don't trust reset_mock - it's tough to convince myself that everything that matters has been reset - are the later tests only passing because of retained state from earlier? What do you think about splitting this test in two parts to cover your version=1 and version=2 cases? Common setup logic could be refactored.
Chris-Peterson444
force-pushed
the
strict-top-level-keys
branch
2 times, most recently
from
5445369
to
991563a
Compare
|
I've made updates per your suggestions @dbungert. I really liked your recommendation for the changes to the stripping function. The filtering function is really nice and made testing pretty simple. Thanks! |
subiquity/server/server.py
Outdated
| self, | ||
| autoinstall_config: dict[str, Any], | ||
| ) -> tuple[dict[str, Any], dict[str, Any]]: | ||
| """Separates autoinstall config by known and uknown keys""" |
There was a problem hiding this comment.
Fixed, thanks!
Chris-Peterson444
force-pushed
the
strict-top-level-keys
branch
3 times, most recently
from
4150e56
to
b7ff1c1
Compare
Chris-Peterson444
force-pushed
the
strict-top-level-keys
branch
2 times, most recently
from
e6f4c70
to
8a35182
Compare
Chris-Peterson444
force-pushed
the
strict-top-level-keys
branch
from
8a35182
to
15f1e73
Compare
Subiquity currently ignores invalid top-level keys, but this has likely
been a major source of confusion of autoinstall capabilities.
In future versions, the following autoinstall config will throw an
AutoinstallValidationError:
version: 2
interactive-sections:
- identity
literally-anything: lmao
This patch adds warnings for version 1 and will begin to throw
an AutoinstallValidationError on these instances in version 2 once
it has been enabled.
Chris-Peterson444
force-pushed
the
strict-top-level-keys
branch
from
15f1e73
to
d812919
Compare
Subiquity currently ignores invalid top-level keys, but this has likely been a major source of confusion of autoinstall capabilities.
In future versions, the following autoinstall config will throw an AutoinstallValidationError:
This patch adds warnings for version 1 and will begin to throw an AutoinstallValidationError on these instances in version 2 once it has been enabled.