Repository navigation
DOCUMENT: Standardizing Contributing Flow with PRs - WIP #62
Description
As previously discussed with @ChristopherA and @gorazdko, we should document the PR opening, reviewing/editing, and merging process. Blockchain Commons does use Github Flow, but often times the PR reviewing process can get cumbersome and/or time-consuming. So, here are some ways to make it work more seamlessly, assuming the contributor is a new contributor:
-
Using Github CLI
gh[Best IMO]:- Contributor forks the repository
- Contributor creates an issue in base repository to start the discussion or a draft PR (for the latter, the branch would have to be created first)
- Contributor signs CLA and either issues a separate PR or sends via email to Christopher
- Contributor creates a new feature-branch from
master - Contributor works on that branch through atomic, signed commits
- Contributor finishes work and issues a PR to
master, checking the "allow edits from maintainers" box - Contributor requests review
- Maintainers can do
gh pr checkout <number>in CLI to review and work on that PR, also through atomic, signed commits. This alleviates Maintainer from doing a review through comments on the PR which the Contributor would later need to manually edit and adjust for, having then to tell Maintainer the review was made, etc. Basically making it simpler and streamlined - Maintainers finish reviewing/editing and can just do
git pushfor all changes to be pushed to Contributor's fork and therefore to the PR as well - If non-maintainers would want/need to work on reviewing/editing that PR, they wouldn't have permission to do
git push. So Contributor would have to invite each non-maintainer to Contributor's fork for them to have the necessary permissions to push directly to that fork and therefore to the PR as well - After the necessary edits and reviews, Maintainers of the base repository can merge to
masterand delete the feature-branch
-
Using Pure Github Flow
- The seven first steps from number one also work here. What changes is beginning on item number eight. Also, here it doesn't matter much whether the reviewer is a Maintainer or a Non-maintainer, so both will be used interchangeably:
- Maintainer branches off of the PR's Contributor branch through a git clone from Contributor fork and branch into a new branch
- Maintainers then work on their branch through atomic, signed commits
- Maintainer finishes working, issues a PR to Contributor's fork
- Contributor merges Maintainer's PR into his fork, updating his own initial PR into base repository
- Maintainer marks base repository PR as ready for merge
- PR can be merged by Maintainer
- Note: this is basically what has been done in PRs for portuguese translation of LBftCL. IMO it's already an improvement on reviewing with comments and requiring Contributor to edit etc.
In any case, we can rapidly notice how 1 is quicker and easier than 2. But it uses gh and requires Contributor to invite non-maintainers to his fork so edits can be streamlined and pushes allowed.
Number 2 doesn't require that, but it does require Contributor to merge a PR into their own fork which effectively creates duplicity in terms of work needed -- merge a PR onto a fork so another PR can be updated, only then to merge that PR onto base repo master. Phew!
But there might also be other ways to go about this.
Thoughts?
Activity
ChristopherA commentedon Jul 19, 2021
There is some GitHub trick method that a contributor can fork the repo, create a branch, and submit PR back that allows the maintainers to directly commit changes to that PR without additional permissions from the contributor. It hasn’t been clear to me how this is done, but is incredibly useful to make it easy to accept useful contributions and encourage/educate the participants, and do final tweaks in them until ready to merge.
ChristopherA commentedon Jul 19, 2021
I would love some form of CI that checks if a CLA from the contributor @github account exists.
namcios commentedon Jul 20, 2021
I might be mixing things, but wouldn't this be achieved with number one? Maintainers can edit and directly commit changes by default. At least for me it's always marked to allow by default. Only for non-maintainers that I'd have to grant additional permissions, for example for another contributor.
namcios commentedon Jul 20, 2021
Some reference links:
https://stackoverflow.com/questions/44030176/how-to-modify-someone-elses-github-pull-request
https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork
https://tighten.co/blog/adding-commits-to-a-pull-request/
ChristopherA commentedon Jul 20, 2021
@namcios seems option 1 might do it. Some testing may be useful.
@shannona any other requirements or ideas?
namcios commentedon Jul 23, 2021
@ChristopherA recently tested it on LBftCL PR 264.
Had to ask PR owner for inviting me into his fork (since I'm not a maintainer of LBftCL) and it worked like a charm with:
gh pr checkout 264gh pushI'm guessing the same would work for a maintainer without the need for invitation?
namcios commentedon Jul 23, 2021
I meant
git pushnotgh pushChristopherA commentedon Jul 23, 2021
Great! @shannona Let’s document this solidly in both translations advice .md and maybe look at the standard CONTRIBUTING.md in the secure template and where appropriate in Community/
@namcios & other interns & volunteers: We welcome any PRs to improve this community wide as likely it will take a while for @shannona to get to it given his two-day a week commitments to Blockchain Commons (mostly Tues/We’d) and a large backlog.
csralvall commentedon Jul 23, 2021
The workflow with quick corrections and fixes of typos will stay as it was or this idea applies to them also?
csralvall commentedon Aug 5, 2021
Hello @ChristopherA , I have a working implementation for this in a private repository, how should I proceed?
ChristopherA commentedon Aug 5, 2021
Great, this will be very useful. For now I’d likely start by adding it to this leaning bitcoin repo, but We could also set up a test repo for it first using our secure template as a start.
I’m not sure what level of repo privileges are required to set this up. maintainer or administrator?
I’m heading out on vacation tomorrow, so before weekend feels difficult. But @shannona coulD work with you next Tuesday on it, or @gorazdko might be able to as well.
csralvall commentedon Aug 10, 2021
For the record:
CLA-signedfrom the target branch of that pr, and fails or succeed depending of that condition. It's not recommended to insert comments from actions to point the error to the requester, because that can create conditions to escalate privileges.[-]Standardizing Contributing Flow with PRs - WIP[/-][+]DOCUMENT: Standardizing Contributing Flow with PRs - WIP[/+]