-
Notifications
You must be signed in to change notification settings - Fork 310
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
Accept Github webhook requests for the Github default branch #1840
Conversation
This allows the webhook to work independent of the name of the default branch on Github (which used to be master, but is now main and can be renamed by the repository owner). Fixes OpenUserJS#1781
While I appreciate the try here... there's a bit more to it than this. I've had a lot of time to consider how to do this without breaking things all over the place. I also wrote a local branch here to do something similar with PAT instead of OUJS GH tokens but haven't had any time to nab some of that code. OUJS requirement is This is fixed requirement for publishing to OUJS since the site was created even before I came here and I concur (as OUJS Admin, Co-owner, and Active Maintainer) that it could be continued to be normalized with an option of Importing scripts is also affected which isn't currently covered in this PR... so this would break that. If you feel you are up to the task of fixing the missing import-ability and setting the UI, constants to the User DB, and the JavaScript wiring within these limits I'll keep this open so you can work on it... otherwise it will be closed until a lot more time is available to address #1781 in entirety. I'm available to interact/review any changes you have if you are willing to be receptive to that. |
Thanks for the response. Yes, I now realize that at least the code at https://github.com/OpenUserJS/OpenUserJS.org/blob/master/libs/repoManager.js needs updating to handle non-master branches.
Can you clarify this a bit? I don't see how the branch name would be relevant to the user preferences right now. Or do you mean there should be a preference to configure the branch name there? Also why must it be exclusive? Why shouldn't it be possible in principle to support both main and master, or even just use the default branch whatever this is? I actually won't have time to work on this, so I'm fine with getting this closed for now. The reason why I opened this PR at all (despite my misconception that it would only involve changing the webhook handler) was that I could not comment on #1781 because discussion there is blocked. And all the technical details you brought up above are not discussed there. So currently this issue does not give any clue that implementing this has some technical difficulties and the short answer before closing the issue looked like there is little interest in getting this fixed. Maybe having some of the technical details added to the discussion there would help future potential contributors to get a better picture of the issue, or maybe the discussion here already helps with this :) To be honest I found the Github webhook setup on OUJS rather complicated or at least not well documented. I can't even find right now in the UI a place where it tells me the webhook URL. When I set this up I think I pulled it out of some forums discussion, and after I had set this up it took me some time understanding why it didn't work. I'm probably just missing something here, but a short info the script editing page on how to setup the Github sync would probably help a lot, and that info should note about the branch name. |
It's in other issue discussions. It is what it is.
A permanent radio/check button on the GH auth method in a users preferences. A one way trip to use
See your first response/question in this comment.
See your first response/question in this comment.
Alright.
Some are in other issues and some of it like I mentioned in the first response, prior to this one, is I've had a lot of time to think about this... along with other indirect security issues that can and will be affected.
Because it's too heated everywhere it has been brought up. Providing a path forward is a better option than discussions about what the history of
There are still technical issues to resolve but the path forward is what I described. If you haven't realized yet we're still in a pandemic and we all don't have a lot of time as you have mentioned. I just attended another funeral this weekend.
It is of lower priority to change this feature, especially around politics, but the assumption you conclude is not true otherwise the issue would have been closed. It is not a fix but a feature as perhaps you have heard before on other projects/life. Reviewing code in a PR is also a good place for discussion as it shows someone has some initiative instead of just being political.
That's because you didn't look hard enough... it's on New Script and New Library right where it has been under Instructions for those with GH auth. Perhaps a read there would be prudent.
The script editing page is definitely not a correct place for that. I've already mentioned where it is and there is a note about using Perhaps it is a good thing that you aren't available to work on this (WIP) because you haven't yet familiarized yourself with the site itself and where to find things. I do appreciate the try but we knew, and I knew, about the default branch in the webhook years ago. As I cited in this PR another issue... "It is almost never as easy as a one liner...". This PR currently is a two-liner. There's a lot more needed and familiarizing ones self with every issue, open and closed, is a good start before a contribution. I do my best to document what I can when I can however I'm not going to write a novel on every issue/PR (normally). I'm open to any other ideas but this one needs great care and extreme caution i.e. tread lightly. So far what I've thought about, and sparsely stated here in the hopes that you were serious about contributing, is how it probably should be done. I have over 20 branches locally of "ideas" that aren't one/two liners. If it's relevant I'll discuss it. The Thank you for trying though. |
Ok, I still don't see why it shouldn't be possible to support more than one name for the primary branch or why migration to main as default would be a one way road. But if you say it has been discussed somewhere in the past let's leave it at that. Hopefully someone will figure out a way to support at least main in the future. Thanks for the feedback. |
This allows the webhook to work independent of the name of the default branch on Github, which used to be master, but is now main. The branch can also be renamed by the repository owner to anything they like, e.g. I have seen teams working with
default
ordevelop
.This PR ensures the webhook works with whatever the main branch is called. Github reports it in the webhook payload as follows (this shows only the relevant details for branch handling, every other detail is omitted):
Fixes #1781