Skip to content
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

Refactor ssg.build_ovals module #10048

Merged

Conversation

jan-cerny
Copy link
Collaborator

@jan-cerny jan-cerny commented Jan 11, 2023

Description:

This is a series of refactoring and improvements in build_ovals.py. These commits have been extracted from PR #10035 making review easier, because 10 commits have to be created to pass CodeClimate.

Then, additional refactoring has been performed, read the discussion below.

Rationale:

@jan-cerny jan-cerny added Infrastructure Our content build system CPE-AL CPE Applicability Language refactoring Improvement which, once completed, will enable the project to progress faster. labels Jan 11, 2023
@github-actions
Copy link

github-actions bot commented Jan 11, 2023

Start a new ephemeral environment with changes proposed in this pull request:

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@vojtapolasek vojtapolasek self-assigned this Jan 12, 2023

xml_content = process_file_with_macros(_path, local_env_yaml)

if not _check_is_applicable_for_product(xml_content, product):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jan-cerny , I see similar piece of code in the function _process_oval_file. Would it be possible to deduplicate some parts of those functions?

Refactor the code by splitting the code into 2 logical blocks
and putting them each in a separate function:
- get_checks_from_benchmark
- get_checks_from_directories
This commit introduces a new command line option `--include-benchmark`.
If this option is specified by the user, OVAL checks from rule
directories in the benchmark directory tree (eg. /linux_os/guide tree)
will be included in the generated OVAL. Adding these files is the
current behavior of the script so it doesn't add a new feature, instead
it allows users to turn this feature off by not providing this command
line option.

This commit also rewords and clarifies the help text for the
"ovaldirs" argument but it doesn't change the behavior of the "ovaldirs"
argument.
The code has many issues that are triggering CodeClimate problems.
We tried to address them by simple refactoring, but then we realized
that more refactoring would make the code more readable and
understandable.

There are 2 branches of the code logic:
1. processing OVAL checks from benchmark directories (eg. /linux_os/guide)
2. processing OVAL checks from shared directories (eg. /shared/checks)

These 2 logic branches have similar code and it's possible to unify
them. However, I found the unification easier when I change the code
to a class.
@jan-cerny
Copy link
Collaborator Author

@vojtapolasek Thanks for the excellent suggestion! I have realized that there were similar blocks of code that differ only in small details. The difference was that some code processed OVALs from benchmark (ie. from rule directories) and other code processed OVALs generated from templates or from /shared. Therefore I have proceed with further refactoring. My outcome is that the whole code is now a class.

@codeclimate
Copy link

codeclimate bot commented Jan 20, 2023

Code Climate has analyzed commit 682892a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 0.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 49.3% (-0.4% change).

View more on Code Climate.

Copy link
Collaborator

@vojtapolasek vojtapolasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the improvements and tests as well.

@vojtapolasek vojtapolasek merged commit 1407b6d into ComplianceAsCode:master Feb 8, 2023
@Mab879 Mab879 added this to the 0.1.67 milestone Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CPE-AL CPE Applicability Language Infrastructure Our content build system refactoring Improvement which, once completed, will enable the project to progress faster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants