-
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
Integration of the OVAL object model into the combine_ovals.py
script
#11236
Integration of the OVAL object model into the combine_ovals.py
script
#11236
Conversation
Skipping CI for Draft Pull Request. |
1ea3fb0
to
66e4e20
Compare
/test all |
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.
It looks amazing, I like the way the model clicks into combine_ovals.py.
ssg/build_ovals.py
Outdated
|
||
def _read_oval_file(self, file_path, context, from_benchmark): | ||
if not file_path.endswith(".xml"): | ||
logging.warning("File '{}' is't ends with '.xml'.".format(file_path)) |
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.
logging.warning("File '{}' is't ends with '.xml'.".format(file_path)) | |
logging.warning("File name '{}' doesn't end with '.xml'.".format(file_path)) |
ssg/build_ovals.py
Outdated
def _read_oval_file(self, file_path, context, from_benchmark): | ||
if not file_path.endswith(".xml"): | ||
logging.warning("File '{}' is't ends with '.xml'.".format(file_path)) | ||
return "" |
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.
Shouldn't it return something different than an empty string? Raise an exception?
|
||
|
||
def parse_args(): | ||
p = argparse.ArgumentParser() | ||
p.add_argument( | ||
"--build-config-yaml", required=True, dest="build_config_yaml", | ||
"--build-config-yaml", |
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.
Although the style updates surely make the code better, they also make the diff larger and therefore the PR becomes harder review. The reviewers when they see the diff they need to evaluate if the change is a code style change or actual behavior change. Next time, please don't do style updates unless necessary or requested by Code Climate. The best commits are small commits that do only necessary things.
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.
Okay, I've moved these changes to at least a separate commit.
build-scripts/combine_ovals.py
Outdated
|
||
return p.parse_args() | ||
|
||
|
||
def setup_logging(args): |
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.
Maybe it would be better to pass args.log
instead of args
.
143c3fa
to
f5187ad
Compare
The CI fail on Fedora latest is now caused by upgrade of the underlying image to f39. The problem has been fixed by #11256. Please rebase on the top of the latest master branch to bring the changes in and unblock the job. |
f5187ad
to
9d369ba
Compare
@jan-cerny Rebased, let's see if CI passes. |
Code Climate has analyzed commit 9d369ba and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 81.5% (50% is the threshold). This pull request will bring the total coverage in the repository to 58.8%. 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.
I have built RHEL9 content from this PR's branch and from the current upstream master branch and I have compared the built oval-unlinked.xml
from both builds and I haven't noticed any major offenses, the differences were different order of attributes and explicit setting of some attributes to the default values. In terms of content validity and these changes in the built OVAL are fine and they shouldn't have impact on the meaning of the checks therefore I won't insist on making the artifacts identical.
The fail of the Ansible lint job is caused by upgrade of the job to Fedora 39 which brings a different rule set in the linter and therefore isn't caused by changes in this PR.
In general, I find this integration successful.
Description:
This PR integrates the use of the OVAL object model into the
combine_ovals.py
file. To do this, thebuild_ovals.py
file is rebuilt and cleaned up, as many functions have been integrated into the object model.Rationale:
This PR is part of the integration of the OVAL object model into the build system.
Review Hints:
To check the functionality of the changes, you can build content using
./build_product
and run a test suite using thetox
command.Depends on: #11235