Add GitHub App support #156
Conversation
MagicRB
force-pushed
the
github_app
branch
3 times, most recently
from
53fa959
to
161a731
Compare
|
I have verified all 3 current backends, Gitea, GitHub legacy and GitHub app. There are a few things left open:
but aside from those things, the code is functional and ready for review |
|
Ah. So it's the avatar where it doesn't render the secret properly and instead only load it on initialize. This can be fixed with a patch, that will hopefully also be accepted upstream. |
|
I'm not quite sure what you mean, but the problem with avatars is simply that I have no clue what token to feed into it. It won't accept the JWT app token (which is what is being fed in now) and the installation tokens are per installation, so again I'm not sure which one to feed in. |
|
I would expect the installation token to work. However unlike other parts of github it doesn't use the secrets api of buildbot and instead expects the token itself -> this needs to be fixed in buildbot. |
|
The status push also just expects a token, it is poasible to hack together. But yes the big problem is the installation token doesn't work. But I've been told that there is a public endpoint we could use, just don't how fine that is ToS. (github.com can render avatars without login, so it must exist somewhere) |
|
https://docs.github.com/en/rest/users/users?apiVersion=2022-11-28#get-a-user that endpoint requires a installation token, i have absolutely no clue how to associate installation tokens with specific users though. I will look into bypassing that endpoint, user avatars should be public in the api |
|
This PR should be ready, I utilized |
I tested the legacy option with the token and it worked fine. I also did get a helpful warning when I used your branch that pointed me in the right direction about what I needed to update on my configuration.
|
|
Wonderful! Thanks for the test. Then I think we're good to merge. I will open more PRs, specifically one to implement webhook app refresh and getting rid of the timer. And another series of PRs getting rid of the horrible hacks i put inplace to workaround buildbots github implementation once i upstream things into buildbot (and figure out a good API, that would be backwards compatible) |
|
I have a look on my system. |
|
Thanks for the fox @Mic92 im currently on my way home. |
|
@mergify rebase |
Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
✅ Branch has been successfully rebased |
|
I added an assertion statement because if you were still using the old repository labels, than there would be no "installation_id" set. Also would this not also break if we just install an app globally to all repositories? |
|
I also forgot to specify app permission when creating the application. Please verify the ones I set are correct. |
|
Looks like we still miss one: |
| Enable the following permissions: | ||
| - Contents: Read-only | ||
| - Metadata: Read-only | ||
| - Commit statuses: Read and write |
There was a problem hiding this comment.
I have:
- commit statuses: RW
- contents: R
- issues: RW
- merge queues: RW
- metadata: R
- webhooks: RW
There was a problem hiding this comment.
I'm sure webhooks are needed for creation, the rest are kind of vague, ill check the endpoints.
going through our code, at least:
- commit statuses: RW
- metadata: R
- webhooks: RW
- contents: R
are needed, not sure about merge queues and issues, ill disable and attempt to get an error
There was a problem hiding this comment.
Just tested CI on a PR without merge queues or issues and it seems to be good, so the only thing we're missing are webhooks so we're fine
|
I just had to delete |
Looks like it's no longer expiring. So it must have something to do with making it public. |
That is a bug, can you provide some logs or something? It should auto renew everything |
I could try to reproduce it, when converting the numtide instance. |
|
I will go over the renewal code, trying to spot a mistake somewhere. Or maybe evict the cache in case of an auth failure |
|
I couldn't find anything obvious, I am testing a "if any project is missing an installation_id, refresh projects" fix though. Will push that once I've verified it works. It should improve the Legacy -> App migration experience |
|
Could we bust caches somehow that are missing |
|
that is what im attempting to implement |
Signed-off-by: magic_rb <richard@brezak.sk>
…son` extension Signed-off-by: magic_rb <richard@brezak.sk>
Signed-off-by: magic_rb <richard@brezak.sk>
|
I fixed a few more things, migration is now seamless both ways, |
|
I think we're good to merge, let's see what things and where they explode, hopefully nowhere |
|
@Mic92 is this what you ran into? if yes, then fixed by #179 |
broke treefmt with this one, apologies, we should setup a pre commit hook or something like that. I'll look into it, so that I don't forget to check for treefmt :) |
Signed-off-by: magic_rb <richard@brezak.sk>
Yes. That's the one. |
Fix `treefmt` errors introduced in #156
This is still work in progress, avatars are broken and the code is ugly. But functionally its complete.