feat!: Improve justification output for provenance_l3_check #29
Conversation
| Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement bot added the OCA Required 
| if failed or skipped: | ||
| result_value = CheckResultType.FAILED | ||
| | ||
| if passed: | ||
| check_result["justification"].append("Successfully verified level 3: ") | ||
| else: | ||
| check_result["justification"].append("Failed verification for level 3: ") |
There was a problem hiding this comment.
I think result_value = CheckResultType.PASSED is missing when verification is successful?
Also, if passed is not empty, this would override the results even if failed or skipped are not empty.
| if failed or skipped: | |
| result_value = CheckResultType.FAILED | |
| if passed: | |
| check_result["justification"].append("Successfully verified level 3: ") | |
| else: | |
| check_result["justification"].append("Failed verification for level 3: ") | |
| if failed or skipped: | |
| result_value = CheckResultType.FAILED | |
| check_result["justification"].append("Failed verification for level 3: ") | |
| else: | |
| result_value = CheckResultType.PASSED | |
| check_result["justification"].append("Successfully verified level 3: ") |
| return result_value | ||
| | ||
| check_result["justification"].append("Could not verify level 3 provenance.") | ||
| return CheckResultType.FAILED |
There was a problem hiding this comment.
| return CheckResultType.FAILED | |
| return result_value |
There was a problem hiding this comment.
I applied these changes in 155b695, thank you.
| Is there any particular reason to use |
| I used |
Yes, that makes sense. But because we haven't released Macaron yet, we don't need to add |
behnazh-w removed the OCA Required oracle-contributor-agreement bot added the OCA Verified | PASSED = "verify passed" | ||
| FAILED = "verify failed" | ||
| ERROR = "verify error" |
There was a problem hiding this comment.
Can you please add comments for these three types to clarify how they are concluded/differentiated?
| # TODO: During verification, we need to fetch the workflow and verify that it's not | ||
| # using self-hosted runners, custom containers or services, etc. | ||
| all_feedback = [] | ||
| all_feedback: list[tuple[str, str, _VerifyArtefactResult]] = [] |
There was a problem hiding this comment.
How about using typing.NamedTuple instead of a plain tuple to improve the readability and prevent potential mistakes in looking up fields?
There was a problem hiding this comment.
That's perfect thanks I didn't know that existed
There was a problem hiding this comment.
Thanks, LGTM. Let's close this PR and move the changes to #46 as discussed.
Check now displays all provenance artefacts which passed, failed, or were skipped. The check now fails when _any_ artefact mentioned in the provenance fails to verify, or
ailrst force-pushed the improve-checks branch from ed9c77d to 78b65b2 Compare 2 participants
Check now displays all provenance artefacts which passed, failed, or were skipped.
The check now fails when any artefact mentioned in the provenance fails to verify,
or must be skipped.