Runnable and Suite configuration support #6006
Merged
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
clebergnu
added
the
customer:Passt
clebergnu
force-pushed
the
runnable_and_suite_config
branch
from
57a2566
to
5d01bf7
Compare
richtja
requested changes
Aug 22, 2024
There was a problem hiding this comment.
Hi @clebergnu, thank you for your work. I have one comment related to config initialization which IMO needs to be resolved and one about deprecation of from_avocado_config which doesn't needs to be resolved here, but if won't, we need to create an issue about it.
clebergnu
force-pushed
the
runnable_and_suite_config
branch
from
5d01bf7
to
8cf2a13
Compare
|
Hi @richtja, thanks for the review. I've addressed both issues, and explained the additional changes to the verification of values in the commit message, reading: |
clebergnu
force-pushed
the
runnable_and_suite_config
branch
from
8cf2a13
to
e34c2e4
Compare
richtja
requested changes
Aug 22, 2024
There was a problem hiding this comment.
Hi @clebergnu, thanks for the update. I just noticed that Runnable.from_avocado_config is also used in sysinfo plugin, can you please update that as well, so we won't use deprecated code in avocado plugins. Thanks
Signed-off-by: Cleber Rosa <[email protected]>
The Runnable.get_configuration_used_by_kind() is the right place to return all configuration used by a runnable, and that includes the global configuration used by all runners. Signed-off-by: Cleber Rosa <[email protected]>
There's a warning that should be given to users passing configuration that is not used by runnables of a specific kind. This refactors that into its own method, and adds tests for it. Signed-off-by: Cleber Rosa <[email protected]>
The behavior of setting the filtered configuration should be applied at every single instantiation. Let's unify that at the class' __init__ method. Signed-off-by: Cleber Rosa <[email protected]>
Since it provides the same signature as the main class initilization method. Internally, let's switch to it, while still allowing people that is creating their own runnables time to switch. Signed-off-by: Cleber Rosa <[email protected]>
Right now, there's a conflict of interest on the "config" attribute of a runnable: it serves as both a convenience holder of default values from Avocado's configuration (default values set by the settings API itself or in current configuration files), and the actual configuration defined in the runnable API. This introduces a different config holder, one with the default configuration known at the time the class is istantiated. At this point, it's a separate entity, but it will cooperate with config, so that the config attribute will provide either the value set in the runnable itself or in the default config. Signed-off-by: Cleber Rosa <[email protected]>
This change allows for the a runnable configuration and the suite and default configuration to coexist with the correct behavior. In short, if the suite has a configuration, it will become the new default configuration for a runnable, while its own configuration will not be touched. Because the purpose of the default_config and config are different, the checks for the values they may contain are different. The default_config should contain the exact keys as in the used config. The config itself, on the other hand, may contain nothing, or some values, but it should never be more than a subset of the config. Note: there's an opportunity here for changing the data structure used for the "config" attribute, one that looks up the value in the "default_config" if it doesn't exist in the actual "config". But, given that the size of the "default_config" will always be very small (given that it's filtered to contain only the used configuration by the runner) it seemed an unnecessary optimization at this time. Fixes: avocado-framework#5998 Signed-off-by: Cleber Rosa <[email protected]>
clebergnu
force-pushed
the
runnable_and_suite_config
branch
from
e34c2e4
to
7608215
Compare
Good catch! Just updated it! Thanks! |
richtja
approved these changes
Aug 22, 2024
|
Merging based on @richtja approval and CI status. |
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 change allows for the a runnable configuration and the suite and default configuration to coexist with the correct behavior.
In short, if the suite has a configuration, it will become the new default configuration for a runnable, while its own configuration will not be touched.
Fixes: #5998