-
Notifications
You must be signed in to change notification settings - Fork 43
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
Github provider: Add support for checks API #3352
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Mixed Scripts Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Mixed Scripts Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM, but curious where you are going with it
internal/providers/github/common.go
Outdated
@@ -64,6 +64,9 @@ var ( | |||
ErrBranchNotFound = errors.New("branch not found") | |||
// ErrNoPackageListingClient denotes if there is no package listing client available | |||
ErrNoPackageListingClient = errors.New("no package listing client available") | |||
// ErroNoCheckPerissions is a fixed error returned when the credentialed | |||
// identity has not been authorized to use the checks API | |||
ErroNoCheckPerissions = errors.New("missing permissions: check") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have the permission by default with the GH App? I think we don't with the OAuth token we request..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding these APIs so that we can get rid of the comment spam.
Currently, we don't have permissions for the checks, this is why I created a custom error that traps it, so that we can handle runs without them. I think we will have to support running without it, at least for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the checks API is only available to GitHub Apps:
If you aren't building a GitHub App, you might be interested in using the REST API to interact with commit statuses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not to say that we should limit ourselves to the lowest common denominator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is why the OSV evaluator uses commit statuses (the OSV evaluator predates the GH app)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Mixed Scripts Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Mixed Scripts Detected.
Thanks for catching those @rdimitrov I promise not to code without my glasses again 😄 |
This commit adds support for the checks API in the github provider. Signed-off-by: Adolfo García Veytia (puerco) <puerco@stacklok.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No Invisible Unicode Characters Detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
This commit adds support for the checks API in the github provider.
Change Type
Mark the type of change your PR introduces:
Testing
Review Checklist: