DOCUMENT: Standardizing Contributing Flow with PRs - WIP

[namcios]

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:

1. 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 push for 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 master and delete the feature-branch

2. 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?

[ChristopherA]

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]

I would love some form of CI that checks if a CLA from the contributor @github account exists.

[namcios]

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.

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]

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]

@namcios seems option 1 might do it. Some testing may be useful.

@shannona any other requirements or ideas?

[namcios]

@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 264
• performed my changes with signed commits, then
• gh push
• changes automatically reflected in the PR

I'm guessing the same would work for a maintainer without the need for invitation?

[namcios]

@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 264
• performed my changes with signed commits, then
• gh push
• changes automatically reflected in the PR

I'm guessing the same would work for a maintainer without the need for invitation?

I meant git push not gh push

[ChristopherA]

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]

The workflow with quick corrections and fixes of typos will stay as it was or this idea applies to them also?

[csralvall]

I would love some form of CI that checks if a CLA from the contributor @github account exists.

Hello @ChristopherA , I have a working implementation for this in a private repository, how should I proceed?

[ChristopherA]

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]

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.

For the record:

• I have setted up a test repo for this, and tested if it works when another account forks the project and requires a pr and it works.
• As I have created that repo, and haven't assigned any role, I have only tested the functioning of the action with administrator privileges.
• The logic is simple: the code checks if the username of the user that request the pull request is inside the directory CLA-signed from 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.