-
Notifications
You must be signed in to change notification settings - Fork 685
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
Do not instantiate Builder() when running Automatus #9755
Do not instantiate Builder() when running Automatus #9755
Conversation
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.
From CPE logic point of view, I can't provide my feedback.
From implementation point of view, platforms_dir
and cpe_items_dir
are being checked in constructor if they are defined - both parameters can be optional. With them being optional, None
don't need to be passed toBuilder
in RuleCheck
.
d29e1ec
to
a792ea4
Compare
I have made |
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.
Implementation looks good to me now.
I have a feeling that the correct way would be to somehow extract this functionality which locatest tests out of the builder class. In the end, Automatus is interested only in function get_all_tests. This function is not related to building content at all. |
The function relies on the Builder class but it would be possible to move it somewhere else and setup the needed stuff there. But then we get to topics like where would it be more suitable or what about code duplication. That's out of scope of this fix. |
@mildas @vojtapolasek Gimme some time, I'm working on a changeset to remove use of Builder from Automatus. |
Then I think that I rather prefer this fix: #9753 |
Extract functions related to fetching the path and loading templated test scenarios. Move them closer to other related functions.
We don't need to instantiate a builder class, we just want the path of the templated test scenarios. The Builder() was a mock anyway.
Rename extracted functions to align with other related ones.
a792ea4
to
960d635
Compare
@mildas @vojtapolasek I have relocated the test scenario functions away from |
tests/ssg_test_suite/common.py
Outdated
template_name = rule_template['name'] | ||
template_vars = rule_template['vars'] | ||
# Load template parameters and apply it to the test case. | ||
template_parameters = templates[template_name].preprocess( |
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.
Wrong variable?
$ python3 tests/test_suite.py rule --libvirt qemu:///session test-suite-rhel8 --datastream build/ssg-rhel8-ds.xml --no-report package_chrony_installed package_ntp_installed
WARNING - You call Automatus using the legacy 'test_suite.py' script, use the 'automatus.py' instead
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/mlysonek/SCAP/content/logs/rule-custom-2022-11-03-1410/test_suite.log
WARNING - Nothing has been tested!
Traceback (most recent call last):
File "/home/mlysonek/SCAP/content/tests/test_suite.py", line 21, in <module>
ret = automatus.main()
File "/home/mlysonek/SCAP/content/tests/automatus.py", line 498, in main
options.func(options)
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/rule.py", line 665, in perform_rule_check
checker.test_target()
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/oscap.py", line 699, in test_target
self._test_target()
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/rule.py", line 445, in _test_target
test_content_by_rule_id = self._get_test_content_by_rule_id(
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/rule.py", line 431, in _get_test_content_by_rule_id
rule_test_content = self._get_rule_test_content(rule)
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/rule.py", line 413, in _get_rule_test_content
all_tests = self._load_all_tests(rule)
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/rule.py", line 403, in _load_all_tests
templated_tests = common.load_templated_tests(
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/common.py", line 498, in load_templated_tests
test = load_test(path, template, local_env_yaml)
File "/home/mlysonek/SCAP/content/tests/ssg_test_suite/common.py", line 508, in load_test
template_parameters = templates[template_name].preprocess(
NameError: name 'templates' is not defined. Did you mean: 'template_vars'?
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.
Nope, it is a global variable that I overlooked.
It is declared in ssg.templates
and Builder()
generates a dictionary during __init__()
.
https://github.com/ComplianceAsCode/content/blob/master/ssg/templates.py#L32
It looks like we cannot easily get rid of Builder()
, :(
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.
So Builder()
goes through all of the templates in _SHARED_TEMPLATES
and pre-loads all of them, this makes sense when building content.
For Automatus case, there is no need to load all of the templates whenever we are testing a templated rule. Aditionally, Builder()
was being instantiated for each tested rule, so this probably improves performance a bit.
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.
7d7fe67 loads the template and preprocesses it.
Load the template from the directory, and process its parameters. This way it doesn't need to rely on global variable ssg.templates.Builder(). Builder() loads all templates, but Automatus doesn't need to do so.
Code Climate has analyzed commit 7d7fe67 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 46.7% (0.1% change). View more on Code Climate. |
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.
Looks good to me. I've tested it with different combinations of Automatus and everything seems fine except #9762 - I was able to reproduce the issue also on 0.1.64 so it's not a regression and doesn't prevent us from merging this PR.
Description:
Builder()
class.Builder()
just for the sake of getting the test scenarios and their paths.Do not try to generateProductCPEs()
whenplatform_dirs
orcpe_items_dir
are not provided.Rationale:
Builder()
to run the test scenarios.Automatus doesn't need to generateProductCPEs()
to run test scenarios, they are only used when building rules into data stream.This avoids need to provideplatform-dirs
andcpe-items-dir
when Automatus doesn't useProductCPEs()
for anything.TypeError: __init__() missing 2 required positional arguments: 'platforms_dir' and 'cpe_items_dir'
#9752Review Hints:
ssg.template.Builder()
when loading test scenarios in Automatus #9753