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.

Sign up for GitHub

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

Jump to bottom

Introduce a Product class, make the project work with it #10529

Merged
merged 7 commits into from
May 17, 2023
Merged

Introduce a Product class, make the project work with it #10529

merged 7 commits into from
May 17, 2023

Conversation

matejak
Copy link
Member

@matejak matejak commented May 5, 2023 •
edited
Loading

Description:

The project uses the concept of "product yaml" data - essentially product properties s.a. name, package manager and so on. Up until now, the format was a simple dictionary, which was enough to hold the data and to expose them later, but as products can have a lot of properties that can be

  1. shared between products, and that are
  2. fully known only at runtime, e.g. because they depend on a package version,

a smarter approach is needed.
See https://complianceascode.github.io/template/2023/04/25/mrac.html#go-declarative for context - the aim is to be able to define and use product properties to be able to write e.g. {{% if product.prefers_single_file_sshd_configuration %}} instead of {{% if product in ("rhel8", "rhel9", "ol8", ...) %}}.

Rationale:

A new class Product has been introduced, and it mimics a dictionary for the time being.
If this gets accepted, the next goal would be to introduce files that are read during the build process that allow to specify m:n product-property relationship.
As of now, this mapping is partially present in ssg/constants.py, and I am sure you will agree that source code is no place for defining what should be the default grub2 path, and so on.

Review Hints:

  • Check out tests for an idea of what regressions are ruled out.
  • Majority of the changes were around determination of product directory - from examining the yaml path to opening the file and looking into the data.
  • That code should either receive the product directory directly as an argument, or it should indeed read the yaml - examining the filename is the worst possible combination.
  • The aim of this PR is to make changes under the hood without introducing regressions.

@matejak matejak added this to the 0.1.68 milestone May 5, 2023
@github-actions
Copy link

github-actions bot commented May 5, 2023

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@matejak matejak force-pushed the product_attributes branch 6 times, most recently from b5adee0 to 1016902 Compare May 5, 2023 15:18
@Mab879 Mab879 added the Infrastructure Our content build system label May 8, 2023
@matejak matejak force-pushed the product_attributes branch 2 times, most recently from 41de622 to e9c5b67 Compare May 9, 2023 09:45
@matejak
Prepare for the concept of compiled products
baa4d24
@matejak
Make product dict an object
21cbef5 
The object is a drop-in replacement for the dictionary,
but it can save and load itself,
and will be extensible easier.
@matejak
Make the product class usable by the build system
cac719d
@matejak
Use a dictionary as the env_yaml argument
b1fa17b 
Formerly, the product_yaml contents were used,
which broke after it ceased to be a dictionary.
@matejak matejak mentioned this pull request May 11, 2023
Merged
jan-cerny
jan-cerny approved these changes May 15, 2023
View reviewed changes
build-scripts/compile_product.py

import ssg.products


Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the pros and cons of having this a separate script versus including this to compile_all.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that before the functionality is stable, having it as a fast and separate step is convenient for playing around with. I don't think that the separate step causes any problems or notable inefficiencies.

@yuumasato yuumasato assigned yuumasato and unassigned yuumasato May 15, 2023
yuumasato
yuumasato reviewed May 15, 2023
View reviewed changes
Copy link
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.

Looks good, :) just made a few minor comments.

build-scripts/compile_all.py Outdated Show resolved Hide resolved
cmake/SSGCommon.cmake Outdated Show resolved Hide resolved
cmake/SSGCommon.cmake Show resolved Hide resolved
@matejak
Streamline compile_all
d7bb804 
As product_yaml is a required argument, don't handle the possibility of
not being supplied
vojtapolasek
vojtapolasek approved these changes May 16, 2023
View reviewed changes
Copy link
Collaborator

@vojtapolasek vojtapolasek 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 the PR looks good. I did a review and it makes sense, the build process works as expected, the compiled product.yml looks sane.

@matejak
Improve the structure of cmake macros
136d279 
Move macros that define build steps that compile source code into
compiled artifacts to a separate macro.
@codeclimate
Copy link

codeclimate bot commented May 17, 2023

Code Climate has analyzed commit 136d279 and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@yuumasato yuumasato self-assigned this May 17, 2023
@yuumasato yuumasato merged commit 9821105 into ComplianceAsCode:master May 17, 2023
@vojtapolasek vojtapolasek mentioned this pull request May 18, 2023
Open
@matejak matejak mentioned this pull request May 22, 2023
Merged
@marcusburghardt marcusburghardt added the Highlight This PR/Issue should make it to the featured changelog. label Jun 14, 2023
matejak added a commit to matejak/scap-security-guide that referenced this pull request Jun 20, 2023
@matejak
Updated fix_rules to use product class
165678b 
Fix fallout from ComplianceAsCode#10529
@matejak matejak mentioned this pull request Jun 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vojtapolasek vojtapolasek vojtapolasek approved these changes

@yuumasato yuumasato yuumasato approved these changes

@jan-cerny jan-cerny jan-cerny approved these changes

Assignees

@yuumasato yuumasato

Labels
Highlight This PR/Issue should make it to the featured changelog. Infrastructure Our content build system
Projects
None yet
Milestone
0.1.68
Development

Successfully merging this pull request may close these issues.
6 participants
@matejak @vojtapolasek @yuumasato @jan-cerny @Mab879 @marcusburghardt