Skip to content

Extend SSG with functions to manage variables#12717

Merged
vojtapolasek merged 11 commits into
ComplianceAsCode:masterfrom
marcusburghardt:ssg_variables
Dec 18, 2024
Merged

Extend SSG with functions to manage variables#12717
vojtapolasek merged 11 commits into
ComplianceAsCode:masterfrom
marcusburghardt:ssg_variables

Conversation

@marcusburghardt
Copy link
Copy Markdown
Member

@marcusburghardt marcusburghardt commented Dec 13, 2024

Description:

Variables are processed differently depending on the type of content and the existing functions are heavily dependent of SCAP, making it hard to process variables in other contexts. For example to use in OSCAL files.

This extension provide the necessary functions for an easier integration with other tools.

Rationale:

Sometimes it is necessary to process the variables and the respective options taking into account the selected options in control files and profiles. These functions are independent of SCAP format.

Review Hints:

I implemented the minimal necessary functions that should be enough to collect the variables information. More functions can be included on-demand. Here is a simple script that can be used to test the results:

import os
from ssg.products import get_all
from ssg.variables import (
    get_variables_by_products,
    get_variable_values,
)
from pprint import pprint

def _get_root_content_dir() -> str:
    # Update the path according to your environment
    home_dir = os.path.expanduser("~")
    content_root_dir = os.path.join(home_dir, "CaC", "Forks", "content")
    return content_root_dir


def get_products(content_root_dir: str) -> dict:
    products = get_all(content_root_dir)
    return products


def validate_products(products: dict, rhel_products: list) -> bool:
    if rhel_products in products:
        return True
    return False


def main():
    rhel_products = ['rhel8', 'rhel9', 'rhel10']
    content_root_dir = _get_root_content_dir()
    products = get_products(content_root_dir)
    
    missing_products = [product for product in rhel_products if product not in products.linux]
    if missing_products:
        print(f"Check the existence of these products: {missing_products}")
        exit(1)

    profile_variables = get_variables_by_products(content_root_dir, rhel_products)
    #pprint(profile_variables)
    profile_variables_values = get_variable_values(content_root_dir, profile_variables)
    pprint(profile_variables_values)


if __name__ == "__main__":
    main()

@marcusburghardt marcusburghardt added enhancement General enhancements to the project. Infrastructure Our content build system labels Dec 13, 2024
@marcusburghardt marcusburghardt added this to the 0.1.76 milestone Dec 13, 2024
@marcusburghardt
Copy link
Copy Markdown
Member Author

marcusburghardt commented Dec 13, 2024

I am working on Python unit tests and will provide an update soon.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 13, 2024

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@vojtapolasek vojtapolasek self-assigned this Dec 13, 2024
@marcusburghardt
Extend ssg with functions to manage variables
7e4b5ef 
Variables are processed differently depending on the content and the
existing functions are heavily dependent of SCAP, making it hard to
process variables in other contexts such as OSCAL. This extension
provides the necessary functions for an easier integration with other
tools.

Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Minor update to make paths easier to be read
ad62b5c 
Remove unnecessary "." from paths.

Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Minor updates related to style guide
161e8e0 
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Include unit tests for exposed functions in variables.py
538ace9 
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
vojtapolasek
vojtapolasek requested changes Dec 13, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Hello,
thank you for this enhancement. I found some small things worth fixing, see other comments.
I used the script from the PR description and it seems it works.
However, do you plan to supply unit tests for this file?
Could you please test at least public functions in the file?
Unit tests would prevent regressions in the future.

Comment thread ssg/variables.py Outdated
Comment thread ssg/variables.py Outdated
Comment thread ssg/variables.py Outdated
Comment thread ssg/variables.py Outdated
Comment thread ssg/variables.py Outdated
@marcusburghardt
Rename id to profile_id to avoid conflicts
2573bb7 
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Simplify function by changing default
11d0a39 
Thanks @vojtapolasek

Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Unify functions to collect variable options
ba78ede 
The former two functions were pretty similar. With this approach code
duplication is avoided and one single function make it simpler in this
context.

Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Update Python unit tests after removing function
38f80a0 
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Copy link
Copy Markdown
Member Author

marcusburghardt commented Dec 13, 2024

Hello, thank you for this enhancement. I found some small things worth fixing, see other comments. I used the script from the PR description and it seems it works. However, do you plan to supply unit tests for this file? Could you please test at least public functions in the file? Unit tests would prevent regressions in the future.

Hi, I already provided the Unit tests but probably you started reviewing some minutes before the relevant commit. : ) I also incorporated your feedback. Could you take a look again, please?

@marcusburghardt
Copy link
Copy Markdown
Member Author

marcusburghardt commented Dec 13, 2024

Ok. I need to take a better look on failed tests for Suse and Ubuntu, but can't continue today.

@marcusburghardt
Fix compatibility issues with older python versions
ebe2d92 
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@vojtapolasek
Copy link
Copy Markdown
Collaborator

vojtapolasek commented Dec 13, 2024

@marcusburghardt thank you for adding unit tests. I am looking at them and comparing to other unit tests. In the content project, usually test data are defined outside the actual test. You define them directly in the test... please what is the idea behind this approach?
I must say that for me it seems that defining test data outside actual test functions seems more naturel and more readable for me.

@marcusburghardt
Create auxiliary function to setup test files
442cf73 
It avoids code duplication for similar tests. By centralizing the setup of
test files it is also easier to update the tests. For example, in this
new function more options were included for variables and used in tests.

Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
@marcusburghardt
Copy link
Copy Markdown
Member Author

marcusburghardt commented Dec 16, 2024

@marcusburghardt thank you for adding unit tests. I am looking at them and comparing to other unit tests. In the content project, usually test data are defined outside the actual test. You define them directly in the test... please what is the idea behind this approach? I must say that for me it seems that defining test data outside actual test functions seems more naturel and more readable for me.

There are many different cases in unit tests since there is no Style Guide definition for this in the project. The tests I wrote don't use data folder and is not the plan to use. They also have slightly different cases to test and therefore I opted to create one temporary directory for each function. But in the last commit I created an auxiliary function to setup the test files and avoid duplication. I hope this make it more readable.

@marcusburghardt @vojtapolasek
Apply suggestions from code review
dc6235b 
Co-authored-by: vojtapolasek <krecoun@gmail.com>
@qlty-cloud-legacy
Copy link
Copy Markdown

qlty-cloud-legacy Bot commented Dec 16, 2024

Code Climate has analyzed commit dc6235b and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 5

Note: there is 1 critical issue.

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

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

View more on Code Climate.

@vojtapolasek
Copy link
Copy Markdown
Collaborator

vojtapolasek commented Dec 18, 2024

It looks good now. The Codeclimate is complaining about too high complexity of some functions, but I looked at it and I think that they could be split into smaller chunks, however I am not sure if it would improve overall code readibility. So I am waiving this check.
Thank you Marcus for the implementation.

vojtapolasek
vojtapolasek approved these changes Dec 18, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Looks good now.

@vojtapolasek vojtapolasek merged commit 80a8fa8 into ComplianceAsCode:master Dec 18, 2024
@marcusburghardt marcusburghardt deleted the ssg_variables branch December 18, 2024 13:28
@marcusburghardt marcusburghardt mentioned this pull request Jan 9, 2025
Merged
@marcusburghardt marcusburghardt added the Highlight This PR/Issue should make it to the featured changelog. label Jan 13, 2025
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

@Mab879 Mab879 Awaiting requested review from Mab879 Mab879 is a code owner

@jan-cerny jan-cerny Awaiting requested review from jan-cerny jan-cerny is a code owner

Assignees

@vojtapolasek vojtapolasek

Labels

enhancement General enhancements to the project. Highlight This PR/Issue should make it to the featured changelog. Infrastructure Our content build system

Projects

None yet

Milestone

0.1.76

Development

Successfully merging this pull request may close these issues.

2 participants

@marcusburghardt @vojtapolasek