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: add review bot #341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jclab-joseph
Copy link

@jclab-joseph jclab-joseph commented Sep 15, 2023

Close #340
See also #296 (comment)

Applying this workflow can be automatic review through comments.
It can automatically reproduce builds through Dockerfile and help with reviews.

See sample: jc-lab/shim-review-bot#2

Sample review directory: https://github.com/jc-lab/shim-review-bot/tree/master/sample-repo
(need pre-built efi, sbat.csv, vendor certificate, and Dockerfile.)

@steve-mcintyre steve-mcintyre added the meta Not a review request, but an issue or notice wrt the signing process label Sep 18, 2023
@steve-mcintyre
Copy link
Collaborator

This looks like nice work! :-)

How far are you planning to go with the automation?

@jclab-joseph
Copy link
Author

@steve-mcintyre What more could be possible? Ideas welcome!

@aronowski
Copy link
Collaborator

If this PR is the appropriate place for suggestions, I'd start with analysing the .sbatlevel section in regard to this bug.
The justification for this is that there may still be a demand for using the older binutils, like here.

@jclab-joseph
Copy link
Author

Check the bug mentioned by @aronowski in v0.0.6.
Result Sample: jc-lab/shim-review-bot#2 (comment)

@aronowski
Copy link
Collaborator

Great job! Thanks!

I'm also thinking about some more minor than major things that can be fairly easily implemented and add some quality of life improvements to the applicants' lives.

For instance, some time ago I posted this comment and while I wouldn't even be able to express myself algorithmically in my natural language on how to implement something like an analyzer that prohibits using outdated upstream SBAT entries (grub.debian,1 in this case), maybe something like a verification of the existence of an applicant's explanation of the patches used would be easier to implement?

For instance, let the bot check for files that match the *.patch glob somewhere in the applicant's repo, and, for each match, check if there's a listing of it in the "What patches are being applied and why" section, so that each one must be mentioned and described as a requirement.

@jclab-joseph
Copy link
Author

@aronowski I'm very excited! Thanks for the great idea. I'll think about how to implement it soon.

Note: https://github.com/rhboot/shim-review/blob/7d5085dbb8c390536e54c98f73c212f3a1f28245/docs/reviewer-guidelines.md

@jclab-joseph
Copy link
Author

jclab-joseph commented Sep 20, 2023

v0.0.7: patch list (sample: jc-lab/shim-review-bot#2 (comment))

path: /tmp/comment.txt
write-mode: overwrite
contents: ${{ github.event.comment.body }}
- uses: jc-lab/shim-review-bot@v0.0.8
Copy link

Choose a reason for hiding this comment

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

A bit nitpick, but in the context of a security review I'd suggest pinning on to a - arguably less readable - commit id.
This is to ensure nobody would rewrite the tag on the - external - action repository and sneak in an altered/deceptive review.

I guess same issue with the write-file-action and swap branch name to a commit id.

@aronowski aronowski mentioned this pull request Feb 23, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Not a review request, but an issue or notice wrt the signing process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meta: auto review bot
4 participants