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

Remove shorthand.xml from the build process #9548

Merged
merged 12 commits into from
Oct 10, 2022

Conversation

jan-cerny
Copy link
Collaborator

@jan-cerny jan-cerny commented Sep 22, 2022

Description:

Remove shorthand.xml from the build process, because the tools now generate a proper XCCDF 1.2 file. We will achieve this removal by merging the responsibility of the relabel_ids.py to build_shorthand.py. Then, we will rename build_shorthand.py to build_xccdf.py.

For more details please read the commit messages of every commit.

Rationale:

Simplify the build process, avoid unnecessary writing and loading back, improve naming.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 22, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 22, 2022

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

@github-actions
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

@jan-cerny jan-cerny changed the title Remove shorthand.xml form the build process Remove shorthand.xml from the build process Sep 22, 2022
Instead of creating namespace-less elements and then using hack with
hard-setting the xmlns attribute we should create elements with
the OCIL namespace explicitely set.
Add ability to extract root XML element of the OCIL document
without a need to save the document to a file.
This change moves the responsibility of the `relabel_ids.py` to
the `build_shorthand.py`. In other words, OVAL and OCIL IDs will
be changed when the shorthand is created, and the 2 aforementioned
scripts are merged. The benefit is that we will avoid saving the
content to file and load it back, which can make the build faster.
The script now builds a full XCCDF 1.2 file, it doesn't create
what we used to call "shorthand" in past. To avoid confusion,
it would be better to rename it to describe more its actual purpose.
@jan-cerny
Copy link
Collaborator Author

I have fixed the Code Climate problem, and I have rebased this PR on the top of the latest upstream master branch.

@jan-cerny jan-cerny marked this pull request as ready for review September 26, 2022 13:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Sep 26, 2022
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Can you please update the build system docs with the changes you made?

Rename the CMake macro `ssg_build_shorthand_xml` to
`ssg_build_xccdf_oval_ocil` according its output type
to better describe its purpose. Then, update comments, and
remove outdated comment about shorhtand format and its processing.
@jan-cerny
Copy link
Collaborator Author

I have updated the build system documentation and then I cleaned shorthand from the CMake files.

@jan-cerny jan-cerny requested a review from Mab879 October 4, 2022 08:44
@Mab879 Mab879 self-assigned this Oct 4, 2022
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Just one minor change in the markdown. This should fix the Code Climate issues.

Datastream document,
6. Generating any derived products (such as CentOS and Scientific Linux).

1. Generate SCE content and metadata.
Copy link
Member

Choose a reason for hiding this comment

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

Please use 1. for the numbering here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused, it already uses 1. here, so probably you wanted to say the opposite.

@jan-cerny
Copy link
Collaborator Author

I have [Change list to not use numbers

@Mab879
Copy link
Member

Mab879 commented Oct 5, 2022

I think what is tripping up Code Climate here is that some lists in this file have a space before the - and some don't. I believe that not having the space is preferred and what is enforced by Code Climate.

To fix this issue, you may want to reformat the lists in docs/manual/developer/07_understanding_build_system.md.

@codeclimate
Copy link

codeclimate bot commented Oct 6, 2022

Code Climate has analyzed commit 1816f31 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 40.8% (0.1% change).

View more on Code Climate.

@Mab879 Mab879 added the Infrastructure Our content build system label Oct 6, 2022
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Other than my two minor questions on the Markdown, everything looks good. Content builds just fine even with SCE enabled.

Thanks for the PR.

@jan-cerny
Copy link
Collaborator Author

@Mab879 Using - is intentional, I don't plan to renumber the lists in the docs.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

Waving the code climate test as tets are coming in other PRs.

@Mab879 Mab879 merged commit f77117c into ComplianceAsCode:master Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Our content build system XCCDF12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants