Skip to content

Fix count_oval_object get wrong count#4427

Closed
Sep0lkit wants to merge 3 commits intoComplianceAsCode:masterfrom
Sep0lkit:patch-1
Closed

Fix count_oval_object get wrong count#4427
Sep0lkit wants to merge 3 commits intoComplianceAsCode:masterfrom
Sep0lkit:patch-1

Conversation

@Sep0lkit
Copy link
Copy Markdown

@Sep0lkit Sep0lkit commented Jun 20, 2019 •
edited
Loading

Description:

  • oval_root no point to the right oval document root, so get the wrong count._

Rationale:

  • xccdf documents have check-content-ref: oval and ocil
  • oval_root variable point to ocil and the rule get no test and no objects

@Sep0lkit
Fix count_oval_object get wrong count
bc93800 
oval_root no point to the right oval document root, so get the wrong count.
@openscap-ci
Copy link
Copy Markdown
Collaborator

openscap-ci commented Jun 20, 2019

Can one of the admins verify this patch?

@yuumasato
Copy link
Copy Markdown
Member

yuumasato commented Jun 20, 2019

@openscap-ci ok to test

@yuumasato yuumasato self-assigned this Jun 20, 2019
@yuumasato yuumasato added this to the 0.1.45 milestone Jun 20, 2019
@yuumasato
Copy link
Copy Markdown
Member

yuumasato commented Jun 20, 2019

@Sep0lkit Nice catch! Thanks for the fix.

The objects defined in definitions which are referenced only via extend_definition are not being counted, although the definition itself is linked to a Rule. So I think they should probably be counted?
What do you think?

See for example rule service_chronyd_or_ntpd_enabled, it extends oval:ssg-service_chronyd_enabled:def:1, but its object oval:ssg-obj_service_running_chronyd:obj:1 is not counted.

Would you be interested in fixing this issue too?

@Sep0lkit
Copy link
Copy Markdown
Author

Sep0lkit commented Jun 21, 2019

@yuumasato understand your view, I will try to fix this issue.

@yuumasato
Copy link
Copy Markdown
Member

yuumasato commented Jun 21, 2019

ITOH, if a definition is extended by multiple definitions, the OVAL count will be bloated up.

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Jun 24, 2019 •
edited
Loading

Hello @Sep0lkit! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-24 11:36:14 UTC

@Sep0lkit
PEP 8 issues
c7b9fb8 
resolve pep 8 issues
yuumasato
yuumasato requested changes Jun 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

@Sep0lkit Thanks for updating the PR, and sorry for delay in review.

The script will now list the all OVAL object types used in each rule correctly.

But I have noticed that the numbers listed at thae end are not actually the number of times an object is used/instantiated, but the number of times a rule used an object of that type.

The summary title of Count of used OVAL objects is misleading, could you updated it to Count of rules using the object.

Comment thread utils/count_oval_objects.py
for extend_def in definition.findall(".//extend_definition"):
extend_ref = extend_def.attrib["definition_ref"]
t = get_ext_def_tests(oval_root, extend_ref)
tests += t
Copy link
Copy Markdown
Member

@yuumasato yuumasato Jun 28, 2019

Choose a reason for hiding this comment

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

As far as I know, there shouldn't be difference under the hood, but lets be consistent and use tests.extend()

Comment thread utils/count_oval_objects.py


def get_ext_def_tests(oval_root, def_refs):
t = []
Copy link
Copy Markdown
Member

@yuumasato yuumasato Jun 28, 2019

Choose a reason for hiding this comment

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

Please name the variable clearly, for example extended_tests.

Comment thread utils/count_oval_objects.py
tests.append(test_ref)
for extend_def in definition.findall(".//extend_definition"):
extend_ref = extend_def.attrib["definition_ref"]
t = get_ext_def_tests(oval_root, extend_ref)
Copy link
Copy Markdown
Member

@yuumasato yuumasato Jun 28, 2019

Choose a reason for hiding this comment

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

Here as well, name the variable more significantly.

@matejak
Copy link
Copy Markdown
Member

matejak commented Jul 19, 2019

Hello @Sep0lkit, there are some small suggestions to your PR, so we would love to see more commits from you. Do you still plan to pursue the merge?

@yuumasato yuumasato modified the milestones: 0.1.45, 0.1.46 Jul 22, 2019
@yuumasato yuumasato modified the milestones: 0.1.46, 0.1.47 Sep 2, 2019
@yuumasato yuumasato modified the milestones: 0.1.47, 0.1.48 Nov 5, 2019
@shawndwells
Copy link
Copy Markdown
Member

shawndwells commented Dec 15, 2019

Closing due to approximately 5 months of inactivity.

@Sep0lkit or @matejak , if you'd like to reopen and continue this work, please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuumasato yuumasato yuumasato requested changes

Assignees

@yuumasato yuumasato

Labels

None yet

Projects

None yet

Milestone

0.1.48

Development

Successfully merging this pull request may close these issues.

6 participants

@Sep0lkit @openscap-ci @yuumasato @pep8speaks @matejak @shawndwells