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

Modification of the OVAL linker to use the OVAL object model #11290

Conversation

Honny1
Copy link
Collaborator

@Honny1 Honny1 commented Nov 20, 2023

Description:

This PR modifies the OVAL linker to use the OVAL object model. Using the model reduces the complexity of the OVAL linker and checks are performed on the OVAL object model side.

Review Hints:

To check the functionality of the changes, you can build content using ./build_product.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 20, 2023
Copy link

openshift-ci bot commented Nov 20, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@Honny1 Honny1 added enhancement General enhancements to the project. Infrastructure Our content build system OVAL OVAL update. Related to the systems assessments. labels Nov 20, 2023
Copy link

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch from 95dac10 to 9094b99 Compare November 20, 2023 12:38
@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch 2 times, most recently from 46924ce to 44a9e79 Compare November 20, 2023 14:37
@jan-cerny jan-cerny self-assigned this Nov 20, 2023
@jan-cerny
Copy link
Collaborator

There are frequent typos in commit messages. Try to use spellcheck in vim before committing. Also, the commit messages should be more detailed.

I appreciate that the reformatting change is a separate commit. But, as I expressed in one of your previous PRs, I think that even if the style updates surely make the code better, they also make the diff larger and therefore the PR becomes harder to 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.

Next thing is about the first commit (Add transalate methode for OVAL ovject model). I'm sorry if I misunderstood it, but It seems that it introduces a method that calls a method that doesn't exist and is added later. That should be done in opposite order. Keep in mind the general rule for committing: Every commit has to build and pass the tests. Sometimes it might be hard to follow this rule, but you should at least conceptually strive to.

The generation of OVAL files need to be split out to a separate pull request. The modification of the linked OVAL is already a large change. Adding the generation feature within the same PR makes the PR too complex.

Regarding the generation of OVAL files, I'm quite puzzled that there still exists the build/${product}/checks/oval directory with some files there. I think that the OVAL files generated from the object model would be a replacement for these files.

@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch 2 times, most recently from 5cbad15 to 3871c04 Compare November 20, 2023 17:47
@Honny1
Copy link
Collaborator Author

Honny1 commented Nov 20, 2023

/test all

@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch from c6a62ca to 5a07b5e Compare November 20, 2023 20:13
@Honny1
Copy link
Collaborator Author

Honny1 commented Nov 20, 2023

@jan-cerny I fixed the issues you mentioned in your comment. Let's discuss the generation of OVAL documents for each rule in PR #11291.

Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

@Honny1

Thanks for the update and I like that the second feature has been split out to the new PR and I also like the commits are now more practical.

I think that the large fail of all profiles tests in CI is caused by mysterious disappearing of all check-export elements from the built XCCDF. grep check-export build/ssg-rhel9-xccdf.xml returns nothing. I have seen you have modified add_missing_check_exports so please take a look.

@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch 2 times, most recently from 271633b to 2394b67 Compare November 21, 2023 14:01
@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch from 2394b67 to a66282e Compare November 21, 2023 14:46
@Honny1 Honny1 marked this pull request as ready for review November 21, 2023 17:54
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Nov 21, 2023
@Honny1
Copy link
Collaborator Author

Honny1 commented Nov 21, 2023

@jan-cerny CI is not failing. I hope everything will work as expected.

Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

I have seen that values of ref_id attributes if reference elements within definition elements in OVAL changed from the short rule ID to a OVAL definition ID. For example, <oval-def:reference ref_id="configure_tmux_lock_after_time" source="ssg"/> changed to <oval-def:reference ref_id="oval:ssg-configure_tmux_lock_after_time:def:1" source="ssg"/> Please revert these attributes back. It's pointless to for an element to reference its own direct parent.

@Honny1 Honny1 force-pushed the adapt-oval-linker-for-oval-object-model branch from 006eaa9 to 105bd84 Compare November 22, 2023 13:15
@Honny1
Copy link
Collaborator Author

Honny1 commented Nov 22, 2023

@jan-cerny The attribute in the reference element has been corrected.

Copy link

codeclimate bot commented Nov 22, 2023

Code Climate has analyzed commit 105bd84 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 58.5%.

View more on Code Climate.

@jan-cerny jan-cerny added this to the 0.1.71 milestone Nov 22, 2023
Copy link
Collaborator

@jan-cerny jan-cerny left a comment

Choose a reason for hiding this comment

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

I think this a great step towards simplification of the build process.

I have built the RHEL 9 content and compared the OVALs and DSs of this PR and of master branch. I haven't noticed any significant difference and I haven't experienced any performance impact.

@jan-cerny jan-cerny merged commit f446565 into ComplianceAsCode:master Nov 22, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancements to the project. Infrastructure Our content build system OVAL OVAL update. Related to the systems assessments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants