Skip to content

refactor: refactor interface of base check#513

Merged
nicallen merged 7 commits intostagingfrom
refactor-base-check
Nov 3, 2023
Merged

refactor: refactor interface of base check#513
nicallen merged 7 commits intostagingfrom
refactor-base-check

Conversation

@nicallen
Copy link
Copy Markdown
Member

@nicallen nicallen commented Oct 13, 2023

Refactor BaseCheck.run_check to return all result info as part of return value, rather than passing in and mutating a CheckResult object containing other data irrelevant to run_check.
Refactor SLSA requirement status information in AnalyzeContext to avoid unnecessary replication of generic info about a SLSA requirement, which can be looked up by ReqName when needed instead.

@oracle-contributor-agreement oracle-contributor-agreement Bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 13, 2023
nathanwn
nathanwn reviewed Oct 19, 2023
View reviewed changes
Comment thread src/macaron/slsa_analyzer/checks/base_check.py
nathanwn
nathanwn reviewed Oct 19, 2023
View reviewed changes
Comment thread src/macaron/slsa_analyzer/checks/check_result.py
nathanwn
nathanwn reviewed Oct 19, 2023
View reviewed changes
Comment thread tests/slsa_analyzer/test_slsa_requirements.py Outdated
@nathanwn
Copy link
Copy Markdown
Member

nathanwn commented Oct 19, 2023

You probably need to regenerate the API doc with make docs-api when the PR is finalized.

behnazh-w
behnazh-w reviewed Oct 20, 2023
View reviewed changes
Comment thread src/macaron/slsa_analyzer/checks/check_result.py Outdated
Comment thread src/macaron/slsa_analyzer/analyze_context.py Outdated
# Remove result_tables since we don't have a good json representation for them.
sorted_on_id = []
for res in _sorted_on_id:
# res is CheckResult(TypedDict)
Copy link
Copy Markdown
Member

@behnazh-w behnazh-w Oct 20, 2023

Choose a reason for hiding this comment

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

This comment should be adjusted.

Copy link
Copy Markdown
Member Author

@nicallen nicallen Oct 31, 2023

Choose a reason for hiding this comment

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

Done.

@behnazh-w
Copy link
Copy Markdown
Member

behnazh-w commented Oct 20, 2023 •
edited
Loading

@nicallen It would be good to add a Developer Docs page to describe the check interface. We already have a README.md though to help contributors add a new check. To avoid duplication, we can move the content of the README.md to the RST page and add a link to the README.md?

@behnazh-w
Copy link
Copy Markdown
Member

behnazh-w commented Oct 20, 2023

@nicallen I think it makes sense to change the commit scope to refactor so that it gets added to the release changelog.

@behnazh-w
Copy link
Copy Markdown
Member

behnazh-w commented Oct 20, 2023

You probably need to regenerate the API doc with make docs-api when the PR is finalized.

Sounds good. @nicallen please rebase on staging to include the latest docs updates.

behnazh-w
behnazh-w reviewed Oct 20, 2023
View reviewed changes
Comment thread src/macaron/slsa_analyzer/checks/check_result.py Outdated
@nicallen
refactor: refactor interface of base check
ecaa885 
Refactor BaseCheck.run_check to return all result info as part of return
value, rather than passing in and mutating a CheckResult object
containing other data irrelevant to run_check.
Refactor SLSA requirement status information in AnalyzeContext to avoid
unnecessary replication of generic info about a SLSA requirement, which
can be looked up by ReqName when needed instead.

Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen
chore: change BaseCheck fields to properties.
58d0420 
Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen
chore: add comments to check result dataclass fields.
28a3b64 
Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen
chore: make check result dataclasses immutable.
0d0115d 
Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen
chore: change test_slsa_requirements to test function rather than usi…
f08b722 
…ng unittest.

Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen
chore: adjust outdated comment.
62b163c 
Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen nicallen force-pushed the refactor-base-check branch from 831955a to a28940d Compare October 31, 2023 05:01
@nicallen nicallen changed the title chore: refactor interface of base check refactor: refactor interface of base check Oct 31, 2023
@nicallen
Copy link
Copy Markdown
Member Author

nicallen commented Oct 31, 2023

@nicallen It would be good to add a Developer Docs page to describe the check interface. We already have a README.md though to help contributors add a new check. To avoid duplication, we can move the content of the README.md to the RST page and add a link to the README.md?

I have created an separate issue for this (#535), I don't think we need to do this immediately as part of this PR.

@nicallen
chore: rename CheckResult.get_dict to get_summary and add detail to t…
9a61b7c 
…he comment.

Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
@nicallen nicallen force-pushed the refactor-base-check branch from a28940d to 9a61b7c Compare October 31, 2023 05:22
@nicallen nicallen merged commit 118cad3 into staging Nov 3, 2023
@nicallen nicallen deleted the refactor-base-check branch November 3, 2023 00:23
art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024
@nicallen
refactor: refactor interface of base check (#513)
ce1586b 
Refactor BaseCheck.run_check to return all result info as part of return
value, rather than passing in and mutating a CheckResult object
containing other data irrelevant to run_check.
Refactor SLSA requirement status information in AnalyzeContext to avoid
unnecessary replication of generic info about a SLSA requirement, which
can be looked up by ReqName when needed instead.

Signed-off-by: Nicholas Allen <nicholas.allen@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@behnazh-w behnazh-w behnazh-w approved these changes

@nathanwn nathanwn nathanwn approved these changes

@tromai tromai Awaiting requested review from tromai

Assignees

No one assigned

Labels

OCA Verified All contributors have signed the Oracle Contributor Agreement.

Projects

None yet

Milestone

Code quality and documentation enhanc...

Development

Successfully merging this pull request may close these issues.

4 participants

@nicallen @nathanwn @behnazh-w @tromai