-
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
Rework of cpe_generate.py
#11644
Rework of cpe_generate.py
#11644
Conversation
Skipping CI for Draft Pull Request. |
🤖 A k8s content image for this PR is available at: Click here to see how to deploy itIf you alread have Compliance Operator deployed: Otherwise deploy the content and operator together by checking out ComplianceAsCode/compliance-operator and: |
4ee07e0
to
aef3461
Compare
build-scripts/cpe_generate.py
Outdated
# Lets scrape the shorthand for the list of platforms referenced | ||
benchmark_cpe_names = get_benchmark_cpe_names(args.shorthandfile) |
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 file actually isn't in shorthand format. It's build/${product}/ssg-${product}-xccdf.xml which is a normal XCCDF document. This comment is outdated. We should change the comment and rename all variables accordingly. Also, we need to rename the argument shorthandfile
.
# skip CPE AL platforms (they are handled later) | ||
# this is temporary solution until we get rid of old type of platforms in the benchmark | ||
if cpe_name.startswith("#"): |
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.
Can you check if what the comment says is still the case?
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.
Yes, I think it's still relevant because, for example, OS platforms use CPE identifiers and CPE dictionaries.
build-scripts/cpe_generate.py
Outdated
|
||
# parse oval file | ||
ovaltree = ssg.xml.parse_file(args.ovalfile) | ||
def load_oval(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.
It's better to not pass args down here. Instead, this function should have 2 specific arguments.
build-scripts/cpe_generate.py
Outdated
|
||
# parse oval file | ||
ovaltree = ssg.xml.parse_file(args.ovalfile) | ||
def load_oval(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.
I think the name of this function is misleading, it's load_oval but it actually does many more actions that just loading. Please rename it.
build-scripts/cpe_generate.py
Outdated
# turn IDs into meaningless numbers | ||
translator = ssg.id_translate.IDTranslator(args.idname) | ||
ovaltree = translator.translate(ovaltree) | ||
oval_document = translator.translate_oval_document(oval_document, store_defname=True) |
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.
The IDs aren't turned into "meaningless numbers", they're added prefix, suffix and number, which makes them unique, but not meaningless.
build-scripts/cpe_generate.py
Outdated
|
||
def load_cpe_dictionary(benchmark_cpe_names, product_yaml, 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.
I don't like that the args is passed down in here. I think that the function should have 2 specific arguments instead of the args argument.
build-scripts/cpe_generate.py
Outdated
p.add_argument( | ||
"idname", | ||
help="Identifier prefix" | ||
) |
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.
When we are here can we actually make this a constant please? I think that it's unlikely to ever be anything different than ssg
.
Also, I have noticed that there are frequent typos in your commit messages. I think I pointed out this before in one of your previous PRs. Please employ a spell checker. |
5cc08ba
to
eede727
Compare
eede727
to
dbf1e69
Compare
Code Climate has analyzed commit dbf1e69 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 50.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 59.8% (0.0% 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.
I have reviewed the code and I have reviewed the generated output from the RHEL 8 product, focusing on the CPE OVAL and CPE dictionary.
Description:
This PR reworks the
cpe_generate.py
script. It simplifies the code for future enhancements such as reducing CPE AL in the data stream. The script uses the OVAL object model to process OVAL. And it separates the process of creating the CPE dictionary and the CPE OVAL.Review Hints:
These changes shouldn't change the functionality of the script.