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

feat: populate cac content product name as component title for CPLYTM-380 and CPLYTM-381 #402

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

huiwangredhat
Copy link
Contributor

@huiwangredhat huiwangredhat commented Dec 25, 2024

Description

Populate Cac content product name as OSCAL component title

Fixes # (CPLYTM-380)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • Added the unit test case.

  • Locally test
    I tested it locally. It could create the OSCAL component definition with the title of the Cac content product name. The trestlebot CLI work example as the following:

poetry run trestlebot sync-cac-content --repo-path ~/trestle-repo --dry-run --committer-email [email protected] --committer-name tester --branch main --cac-content-root ~/content --product ocp4 --cac-profile ~/content/controls/nist_ocp4.yml --oscal-profile nist_rev5_800_53

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@huiwangredhat huiwangredhat force-pushed the 217 branch 4 times, most recently from d5458f9 to 183b12f Compare December 25, 2024 08:32
@huiwangredhat huiwangredhat force-pushed the 217 branch 2 times, most recently from 66cd46b to 1e9f047 Compare December 26, 2024 02:15
@huiwangredhat huiwangredhat changed the title feat: populate cac content product name as component title CPLYTM-380 CPLYTM-381: populate cac content product name as component title Dec 26, 2024
@huiwangredhat huiwangredhat changed the title CPLYTM-380 CPLYTM-381: populate cac content product name as component title feat: populate cac content product name as component title Dec 26, 2024
comp_description=component_description,
comp_type=component_definition_type,
filter_by_profile=filter_by_profile,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to create a new component definition directly? I saw an AC item of CPLYTM-217:

If an existing component definition is present in the workspace you have to complete in place updates, it is not a recreation (using trestle sdk tooling)

Copy link
Contributor Author

@huiwangredhat huiwangredhat Dec 27, 2024

Choose a reason for hiding this comment

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

Yes, it will create a new component definition. I think the AC is out of the CPLYTM-217 scope. It seems for whole EPIC. Also, the Fourth AC is not for CPLYTM-217.

The rule and parameter information from yaml files in cac are present as OSCAL properties

If there is an existing component definition in the workplace, I have no the component's uuid and title mapping, it seems that the component element can't be obtained correctly. It's difficult to say the component exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have thought about this issue again. It should be updated. But I don't know how to update it at that moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new function to update the existing component definition.

@@ -35,3 +41,16 @@ def run_bot(pre_tasks: List[TaskBase], kwargs: Dict[Any, Any]) -> BotResults:
),
dry_run=kwargs.get("dry_run", False),
)


def get_component_title(product_name: str, cac_path: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is for CaC content specially, utils module is used for common functions for cli commands. Since there are following work based on this to attach component props, how about moving to a transformation workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. How about move it to a new file trestlebot/transformers/cac_transformer.py?

@gvauter gvauter changed the title feat: populate cac content product name as component title feat: CPLYTM-380 populate cac content product name as component title Jan 2, 2025
@gvauter gvauter changed the title feat: CPLYTM-380 populate cac content product name as component title feat: cplytm-380 populate cac content product name as component title Jan 2, 2025
@huiwangredhat huiwangredhat force-pushed the 217 branch 4 times, most recently from 88422ca to dbaed45 Compare January 3, 2025 08:25
@qduanmu qduanmu requested review from jpower432 and mprpic and removed request for mprpic January 6, 2025 02:37
Copy link

@marcusburghardt marcusburghardt 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 just minor comments but already tested locally and it is working fine.

product = kwargs["product"]
cac_content_root = kwargs["cac_content_root"]
component_title = get_component_title(product, cac_content_root)
component_description = kwargs["product"]

Choose a reason for hiding this comment

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

Not blocking but since we are reading the product properties, we could use the full_name as description. This can be done in a separate PR anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the component description, and it works. But it can't pass the test with python 3.8. The reason is that Python 3.8 and earlier used Tuple[str, str] from the typing module for type hinting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the test error.

with open(ofile, "r", encoding="utf-8") as f:
data = json.load(f)
for component in data["component-definition"]["components"]:
if component.get("title") == get_component_title(

Choose a reason for hiding this comment

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

Could it be compared with oscal_component.title to avoid calling this function twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated.

logger.info("Update the exsisting component definition.")
# Need to update props parts if the rules updated
# Update the version and last modify time
update_component_definition(ofile)

Choose a reason for hiding this comment

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

I assume this function is here without conditions just temporarily, correct? Or is it necessary to update version and timestamp whenever the command is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it should update the rules/parameters part according to the specific requirement. At that moment, I am not sure how to update that part, but the version and last-modified should be updated together with all the updates.

@huiwangredhat huiwangredhat changed the title feat: cplytm-380 populate cac content product name as component title CPLYTM-380 CPLYTM-381 feat: populate cac content product name as component title Jan 6, 2025
Copy link

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the updates.

@huiwangredhat huiwangredhat changed the title CPLYTM-380 CPLYTM-381 feat: populate cac content product name as component title CPLYTM-380, CPLYTM-381 feat: populate cac content product name as component title Jan 6, 2025
@@ -29,6 +29,7 @@ github3-py = "^4.0.1"
python-gitlab = "^4.2.0"
ruamel-yaml = "^0.18.5"
pydantic = "^2.0.0"
ssg = {git = "https://github.com/ComplianceasCode/content"}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the ssg guide, it recommends to use a stable version, it can change unexpectedly thus unstable when installing from master. e.g.,
`https://github.com/ComplianceasCode/[email protected]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. The latest tag of content is v0.1.75, released on 2024 Nov 11. We need the latest SSG extension in the master. Let's keep it now. It could be updated when the new tag contains the latest changes.

authored_comp: AuthoredComponentDefinition = AuthoredComponentDefinition(
trestle_root=working_dir,
)
authored_comp.create_update_cac_compdef(
Copy link
Contributor

Choose a reason for hiding this comment

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

The create or update in this function is just in local, should these updates be pushed to remote like that in the compdef create command? I'd like to see your opinions here @jpower432 and Marcus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the sync_cac_content_task to push the local change to the remote.

@huiwangredhat huiwangredhat changed the title CPLYTM-380, CPLYTM-381 feat: populate cac content product name as component title CPLYTM-380, CPLYTM-381-feat: populate cac content product name as component title Jan 6, 2025
@huiwangredhat huiwangredhat changed the title CPLYTM-380, CPLYTM-381-feat: populate cac content product name as component title CPLYTM-380-CPLYTM-381-populate cac content product name as component title Jan 6, 2025
@huiwangredhat huiwangredhat changed the title CPLYTM-380-CPLYTM-381-populate cac content product name as component title feat: populate cac content product name as component title Jan 6, 2025
@huiwangredhat huiwangredhat changed the title feat: populate cac content product name as component title feat: CPLYTM-380 populate cac content product name as component title Jan 6, 2025
@huiwangredhat huiwangredhat changed the title feat: CPLYTM-380 populate cac content product name as component title feat: populate cac content product name as component title for CPLYTM-380 and CPLYTM-381 Jan 6, 2025
@huiwangredhat huiwangredhat merged commit 371ab6b into complytime:main Jan 7, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants