-
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
Test and use rules to components mapping #10693
Conversation
21aa810
to
5ca7909
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 for the PR!. Please see my comments.
ssg/build_yaml.py
Outdated
|
||
def _process_values(self): | ||
for value_yaml in self.value_files: | ||
value = Value.from_yaml(value_yaml, self.env_yaml) | ||
self.all_values[value.id_] = value | ||
self.loaded_group.add_value(value) | ||
|
||
def __process_rule(self, rule): |
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.
Any reason for the double underscore?
tests/test_components.py
Outdated
if template_name in template_to_component: | ||
component = template_to_component[template_name] | ||
reason = ( | ||
"all rules using template '%s' should be assigned to component " |
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.
"all rules using template '%s' should be assigned to component " | |
"all rules using template '%s' must be assigned to component " |
I think that if this this behavior cannot be overridden using the world "must" or "shall" would be better. This applies to all the reasons in this file.
|
||
def _process_values(self): | ||
for value_yaml in self.value_files: | ||
value = Value.from_yaml(value_yaml, self.env_yaml) | ||
self.all_values[value.id_] = value | ||
self.loaded_group.add_value(value) | ||
|
||
def __process_rule(self, rule): | ||
if self.rule_to_components is not None and rule.id_ not in self.rule_to_components: |
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 motivated mainly by feedback to contributors that forget to assign components, and this case is not caught by tests. A build failure provides an early feedback, and the contributor will have to think of components early on.
However, there are disadvantages as well:
- We may end up with an "other" component, but its rules would not share reasons to change, which is the main driving force behind components.
- Although there is an existing division by package related to files that the check/remediation operates on, this structure again may not be beneficial.
- If a rule should be assigned to more components, this error alone won't assure that.
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 may end up with an "other" component, but its rules would not share reasons to change, which is the main driving force behind components.
We can do it a more clever way, for rules that don't belong to any component we can put them to some predefined categories that aren't neccesary a RHEL component but make sense for the assessment, for example "filesystem", "users", "crypto".
Although there is an existing division by package related to files that the check/remediation operates on, this structure again may not be beneficial.
It's not a problem of this check, it's the problem of the way we did the initial assignment that is already merged to master.
If a rule should be assigned to more components, this error alone won't assure that.
Yes, but it would at least assure that the rule has at least one component. Once the reviewer of the PR will see that a component is modified, he might think about possible other components.
If a rule isn't part of any component, the build will fail. The enforcement will be efficient for the `linux_os` benchmark.
Also introduces a `ssg.components` module to the project which can be used later in other tools and build system to work with component files data. This commit adds a simple test case `test_components.py` which tests that - components don't map to rules that don't exist - all rules are mapped to at least 1 component - the rule is assigned to component according to templates or template parameters
This test checks if a rule is mapped to component if it uses a package platform or other platforms that can be easily used to determine to which component the rule should belong to.
This commit extracts method `__process_rule` to reduce the code complexity of the original method, as suggested by CodeClimate.
This aims to address the CodeClimate code complexity problem in the `main()`.
CodeClimate emits a warning if there is too many return statements in a single function.
This change reduces code duplication.
Products can decide to use components or they can define their own set of components. Products specify a path to the directory with component files by the `components_root` key in the `product.yml`. This allows products like OCP to use components.
Extract function get_rule_to_components_mapping and move it to the ssg.components module in order to simplify the BuildLoader class.
The reason is to avoid code duplication.
This change will add the mapping of a rule to component to the resolved rules. Developers that build the content will be able to see all components that the given rule is assigned to in the resolved rule yamls.
This will prevent everyone from running the script without its parameters.
This will allow developers to add additional tests for component mapping when a component assignement will be depending on the template used in the rule.
If this this behavior cannot be overridden using the world "must" or "shall" would be better. This applies to all the reasons in this file.
I have rebased this PR on the top of the latest upstream master branch. I have updated example product.yml in Developer's guide. I have replaced double underscore by a single underscore. I have add components to resolved rules. I have made the test_components.py paremeters required and I have created testing plugins for the templates. I have also replaced must by should. |
Addressing: loader.process_directory_tree(subdir) File "/__w/content/content/ssg/build_yaml.py", line 1265, in process_directory_tree self._load_group_process_and_recurse(start_dir) File "/__w/content/content/ssg/build_yaml.py", line 1259, in _load_group_process_and_recurse self._process_rules() File "/__w/content/content/ssg/build_yaml.py", line 1383, in _process_rules if not self._process_rule(rule): File "/__w/content/content/ssg/build_yaml.py", line 1372, in _process_rule rule.components = self.rule_to_components[rule.id_] TypeError: 'NoneType' object has no attribute '__getitem__'
I have fixed a traceback a traceback and I have removed a space. |
Avoid code duplication in ssg/components.py. The mapping functions are similar, they differ only in attribute and the type of the dicitionary values. By unifying the code into a single private function we can avoid this duplication.
I have fixed the CodeClimate issue. |
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 for the PR! One minor nitpick.
ssg/components.py
Outdated
return components | ||
|
||
|
||
def __reverse_mapping(components, attribute): |
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.
Double underscore here as well.
/packit retest-failed |
I have changed the underscore |
Code Climate has analyzed commit 5d20bde and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 59.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 52.9% (0.1% change). View more on Code Climate. |
/packit build |
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 for the PR! This will be great addition to the project!
Description:
This PR is a follow up on PR #10609 and has been initially created by extracting some commits from there.
The PR introduces a new module
ssg.components
which is a basis for future use of the component data in our project. A simple unit test for this module is added as well.The PR introduces the test that performs some basic checks that will help keep the component data consistent. This test tests the following facts:
package_installed
,package_removed
,service_enabled
package
platform.The PR enforces component assignment for all rules in the
linux_os/guide
benchmark. That will prevent people from introducing a new rule without mapping it to a component.For more details, please read the commit messages of every commit.
Rationale:
These changes will ensure the component data consistency. Mandatory component assignment will help content authors think about component outreach of new rules. The tests will help people assign the new rules to correct components.
Review Hints:
Remove some rules from some component, eg. remove the rule
package_fapolicyd_installed
fromcomponents/fapolicyd.yml
and runctest --verbose -R components
to see how the test reacts. Also, try to build the content to see an error.