feat: CPLYTM-389 add cac content rules transformation #411
Conversation
qduanmu
changed the title
There was a problem hiding this comment.
Generally LGTM. I think we should wait for the fixing of line163 in cac_transformer.py and do the test, then merge it.
qduanmu
force-pushed
the
cac-rule-transform
branch
2 times, most recently
from
d04094c to
1381bf7
Compare
qduanmu
force-pushed
the
cac-rule-transform
branch
from
1381bf7 to
caaa44d
Compare
The function could work as expected actually, and it has been verified(testing result) by manually setting the jinja macros directory which raised the error, once the ComplianceAsCode/content#12816 is merged, just uncomment the function is enough. |
| compdef.metadata.version = str( | ||
| "{:.1f}".format(float(compdef.metadata.version) + 0.1) | ||
| ) | ||
| compdef.metadata.last_modified = ( |
There was a problem hiding this comment.
Nit: For consistency across the codebase, you could use trestle modelutils instead for this update - https://github.com/oscal-compass/compliance-trestle/blob/9d5ebf3eec7fc95b721a659206905d11d6ffd724/trestle/common/model_utils.py#L662
| cac_content_root: str, | ||
| compdef_type: str, | ||
| oscal_profile: str, | ||
| authored_object: AuthoredObjectBase, |
There was a problem hiding this comment.
Nit: It seems like the authored_object is mainly used to set the trestle_root, it might make more sense to just pass that value in directly.
There was a problem hiding this comment.
Good catch, updated, thanks.
| from processing. | ||
| """ | ||
| super().__init__(working_dir, model_filter) | ||
| repo_path = pathlib.Path(self.working_dir) |
There was a problem hiding this comment.
Nit: The trestle SDK has existing logic for getting the model path that could be useful - https://github.com/oscal-compass/compliance-trestle/blob/9d5ebf3eec7fc95b721a659206905d11d6ffd724/trestle/common/model_utils.py#L352
Signed-off-by: Sophia Wang <[email protected]>
| results = run_bot(pre_tasks, kwargs) | ||
| logger.debug(f"Trestlebot results: {results}") | ||
| except Exception as e: | ||
| traceback_str = traceback.format_exc() |
There was a problem hiding this comment.
NIT: I could be missing something, but this seems to duplicate the logic in the common.handle_exceptions decorator?
There was a problem hiding this comment.
Yes, we have the handle_exceptions decorator, but while I'm working the integration testing, I found it can not exit with the correct exitcode, so I updated the autosync command in #391. I could spend some time on this decorator and see how to improve it in a separate PR.
| def __init__(self, rule_id: str, rule_dir: str) -> None: | ||
| """Initialize.""" | ||
| self._id = rule_id | ||
| self._description = "" |
There was a problem hiding this comment.
NIT: Potentially consider default value of None here?
There was a problem hiding this comment.
Thank you, George, here if using None, it may raise a validation error, because none is not an allowed value for Property.
Description
Fixes CPLYTM-389
Type of change
How has this been tested?
Test Configuration:
Checklist