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

Accept Github webhook requests for the Github default branch #1840

Closed
wants to merge 1 commit into from

Conversation

phw
Copy link

@phw phw commented Oct 16, 2021

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 or develop.

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):

{
  "ref": "refs/heads/main",
  "repository": {
    "default_branch": "main"
  }
}

Fixes #1781

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
@Martii
Copy link
Member

Martii commented Oct 18, 2021

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 master only at this time... if main is supported in the future at the UI at https://openuserjs.org/user/preferences it needs to be a one way trip i.e. no going back to master. There are no other branch names that will be supported at this time.

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 main for GH defaults. However until git itself creates a new repository that is main as the default, out of the box, this is a non-high priority issue atm.

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.

Applies to #1791 #1781

@Martii Martii added the Active Maintainer Veto Active Minion Mode! label Oct 18, 2021
@phw
Copy link
Author

phw commented Oct 19, 2021

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.

OUJS requirement is master only at this time... if main is supported in the future at the UI at https://openuserjs.org/user/preferences it needs to be a one way trip i.e. no going back to master. There are no other branch names that will be supported at this time.

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.

@Martii
Copy link
Member

Martii commented Oct 19, 2021

Can you clarify this a bit?

It's in other issue discussions. It is what it is.

Or do you mean there should be a preference to configure the branch name there?

A permanent radio/check button on the GH auth method in a users preferences. A one way trip to use main and doesn't work/show for non-GH authed accounts.

Also why must it be exclusive?

See your first response/question in this comment.

Why shouldn't it be possible in principle to support both main and master, or even just use the default branch whatever this is?

See your first response/question in this comment.

I actually won't have time to work on this, so I'm fine with getting this closed for now.

Alright.

And all the technical details you brought up above are not discussed there.

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.

#1781 because discussion there is blocked.

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 master is. It is the whole reason why GH changed to main. We are well aware of the politics surrounding it however breaking changes should be minimized and right now it's not a high priority to introduce breaking changes. Also every comment in the code is there for a reason... it's the target goal usually. Changing a default behaviour isn't a good idea which is why our structure exists. I'm the active maintainer so these considerations need to be observed by anyone contributing.

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 :)

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.

and the short answer before closing the issue looked like there is little interest in getting this fixed.

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.

I can't even find right now in the UI a place where it tells me the webhook URL

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.

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.

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 master and the URL. That's part of the missing UI change in this PR as well since the requirement is master as default with a one way trip to main. There are hundreds of issues on many projects, including ours, about people wanting removal of master. As it's a breaking change it will be a choice and each Author will have to live with their choice and the consequences that come with it. We, and especially myself, are attempting to think about how to minimize the consequences. You would have broken a portion of the site had it not been reviewed.

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 master is currently the only way... and changes without support of the Active Maintainer (myself) won't happen along with any other relevant party ranking including the ultimate veto of the Establishing Owner.

Thank you for trying though.

@phw
Copy link
Author

phw commented Oct 19, 2021

It's in other issue discussions. It is what it is.

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
@phw phw deleted the webhook-git-main-branch branch February 2, 2023 13:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Active Maintainer Veto Active Minion Mode!
Development

Successfully merging this pull request may close these issues.

Support github sync main branch
2 participants