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

Add new Branch Protection "Require Last Push Approval" #2574

Closed
fproulx-boostsecurity opened this issue Nov 23, 2022 · 9 comments · Fixed by #2577
Closed

Add new Branch Protection "Require Last Push Approval" #2574

fproulx-boostsecurity opened this issue Nov 23, 2022 · 9 comments · Fixed by #2577

Comments

@fproulx-boostsecurity
Copy link

New Branch Protection features added in October are not yet supported https://github.blog/changelog/2022-10-20-new-branch-protections-last-pusher-and-locked-branch/

https://docs.github.com/en/rest/branches/branch-protection#get-branch-protection
require_last_push_approval

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 23, 2022

Thank you, @fproulx-boostsecurity !

This would be a great PR for any new contributor to this repo or a new Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

@XaurDesu
Copy link
Contributor

Hey there, is this issue still available? I'm currently learning how to contribute to open source, and this was marked as a good place to start.

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 24, 2022

Hey there, is this issue still available? I'm currently learning how to contribute to open source, and this was marked as a good place to start.

Cool! Yes, this issue is available.
Thank you, @XaurDesu , this issue is yours.

@XaurDesu
Copy link
Contributor

Where should this change be located, exactly? I am currently reviewing the code structure of this project, yet I am not really sure where should we add this change. commit cb24cab is concerned with this functionality too and references github/github-accessors.go, github/github-accessors_test.go and github/repos.go, yet i'm not sure if either of those is the target for this change, sorry if this is an amateurish question, then again, i'm pretty new.

@dtdao
Copy link

dtdao commented Nov 24, 2022

@XaurDesu Dang you beat me getting to this ticket. 😄
I was also kinda poking around as well. And i think cb24cab added require_last_push_approval but there still no implementations for lock_branch and allow_fork_synching.

So...i would think you need to update the Protection struct with those 2 new fields.
and following the patterns with the other fields in that struct. Add 2 new get functions GetLockBranch and GetAllowForkSynching . But that just kinda a guess, from a guy trying to learn as well. 😅

cc: @gmlewis

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 25, 2022

Thanks, @dtdao and @XaurDesu - yes, that sounds like a promising plan. Let's please take a look at this in a PR and see how well it works out.

@XaurDesu
Copy link
Contributor

I've got an uncommited version of the changes proposed by @dtdao in my local machine, mostly basing it in the way cb24cab was strtuctured, with all changes being inside repos.go, modifying Protection and adding the type structs AllowForkSyncing and LockBranch tests don't indicate any failures but i'm not sure of the format, should i commit to my fork those changes?

cc. @gmlewis

@gmlewis
Copy link
Collaborator

gmlewis commented Nov 26, 2022

tests don't indicate any failures but i'm not sure of the format, should i commit to my fork those changes?

I'm not sure I understand the question, but feel free to create a PR and add "Fixes: #2574" to the description and we can take a look at it, and see where to go from there. See our CONTRIBUTING.md guide for general helpful advice about contributing to this project.

@XaurDesu
Copy link
Contributor

Just linked a pull request for it, again, it is fairly barebones but has the functions discussed in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants