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

How about letting developers ack PRs? #112

Open
ishanSrt opened this issue Mar 30, 2018 · 11 comments
Open

How about letting developers ack PRs? #112

ishanSrt opened this issue Mar 30, 2018 · 11 comments

Comments

@ishanSrt
Copy link

ishanSrt commented Mar 30, 2018

These PRs can go in a separate queue for maintainers and will definitely not contain trivial errors but may contain design changes. Gitmate can have one more state if PRs are acked by a developer, somewhat like pseudo passed but still not approved for merge.

The more developers ack a PR, the PR can get to one side of the queue, that means it has more probability of being error free. So if someone is not quite in the mood to review, he can start from this end and keep merging.

While those PRs which are acked by lesser developers or (some requested changes and some acked) should go to the other end, as a maintainer will have to look closely and then see why is the PR being controversial.

@ishanSrt
Copy link
Author

@sks444 this may concern you

@sks444
Copy link
Member

sks444 commented Mar 30, 2018

Thanks, @ishanSrt, we will be doing something like this in the gamification project :)
Also, corobo and meta-review project is related to it ;)

@Makman2
Copy link
Member

Makman2 commented Apr 1, 2018

Not sure if having multiple gitmate states is the right solution (I guess it makes things quite complicated, having different states for different GitHub teams), but in general a good idea 👍

@jayvdb
Copy link
Member

jayvdb commented Apr 7, 2018

First question is whether gitmate can support it?

If not, this is a dead issue until gitmate supports it.

If you want it, go hack gitmate :P

Actually we have a gitmate plugins project, so this could potentially be done as part of that project.

@nkprince007
Copy link
Member

Uhm, we could change the context & description of the commit status API (both GitHub & GitLab) stating that it was acknowledged / unacknowledged by so and so no. of developers, while still marking it as pending. And we could mark it as approved when a maintainer acks the PR.

Not sure if we could visualize the queue as requested without making some heavy changes to the GitMate.io UI.

@ishanSrt
Copy link
Author

ishanSrt commented Apr 26, 2018

Not sure if we could visualize the queue as requested without making some heavy changes to the GitMate.io UI.

maybe just make gitmate set extra tags and labels, which can be filtered in GitHub issue search FTTB.

@jayvdb
Copy link
Member

jayvdb commented Apr 26, 2018

The solution definitely needs to use labels which form the basis of the work queues.

I would be happy enough if developer-acked PRs were given the 'process/approved' label, but the check statuses would still be pending until the maintainer also does an ack. Perhaps rather than allow use of ack, developer might use something else, like pack for 'partial ack', or dack for `developer ack'. Or 'ontrack'...

That puts them into the queue for maintainers to pick them up to do final reviews and merges, and also pushes them out of the review queue, so they wont have lots of other useless "LGTM" comments from developers who are trying to use GitHub as a MMORPG.

@jayvdb
Copy link
Member

jayvdb commented May 6, 2018

Another way to do this is replace gitmate ack functionality with the github native review system which now allows a custom list of reviewers. I guess we can set it to require a review from the developers group.

@jayvdb
Copy link
Member

jayvdb commented May 6, 2018

I think GitHub requires a CODEOWERS to let developers approve changes.

@jayvdb
Copy link
Member

jayvdb commented May 6, 2018

coala/cEPs#122

Also should try on docs repo

@jayvdb
Copy link
Member

jayvdb commented May 11, 2018

@ishanSrt , you want to try setting it up for one repo : coala/cEPs#50

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

No branches or pull requests

5 participants