Difference between revisions of "GitHub Pull Request"

From Dogtag
Jump to: navigation, search
(Submitting patches)
(GitHub - Merge and update fork)
 
(58 intermediate revisions by 3 users not shown)
Line 1: Line 1:
 
= Overview =
 
= Overview =
  
Github Pull Request is a way to keep the official repo clean by allowing other developers to review your patch before it gets merged into the corresponding branch. It offers a lot of benefits over the gerrithub. See [[Gerrit vs GitHub Pull Request]] for entire adv/disadvantages
+
Patch review is a way to ensure the patches merged into the source repository are correct and good (i.e. clean, good design, conforming to standards). However, doing a patch review sometimes takes a significant amount of effort, which can take away the time to do other things. As a compromise, the review will be done on important or high-risk patches only.
  
= GitHub - Initial setup =
+
Here are some situations where a patch review is required or highly recommended:
 +
* Patches submitted by external contributors have to be reviewed and merged by a team member.
 +
* Patches for areas that are critical or complex, e.g. crypto changes, replication, serial number allocation.
 +
* Patches for maintenance/stable branch (i.e. anything other than master branch), e.g. 10.5 branch.
 +
* Patches for unfamiliar areas (i.e. mainly written by someone else).
 +
* Patches that will affect test automation in a significant way.
 +
* Patches that were already reviewed but change significantly when cherry-picked into another branch.
  
* Clone the upstream project into a local repository
+
Here are some situations where a patch review is not really necessary:
+
* Patches that do not change significantly when cherry-picked into another branch.
$ git clone https://github.com/dogtagpki/pki.git
+
* Patches that are simple and low-risk, e.g. typos, log message changes. High-risk patches should still be reviewed even though it's trivial, e.g. crypto changes.
 +
* Patches that refactor/reorganize the code without changing the functionality. These patches should be done in simple steps so others can understand easily. Do not make major/complex changes in a single patch.
 +
 
 +
There are various ways to do patch reviews, e.g. in person, over IRC, via email, in the ticket, but the recommended way is with GitHub Pull Request. GitHub PR allows patch reviews to be done conveniently online. All PRs can be found in one place. The patches are displayed in an easy-to-read format. Anybody can provide a comment on the patch in general or on a specific code in the patch. The CI will run automatically. And finally, the patches can be rebased and merged with a single click.
 +
 
 +
There is no record keeping required for patch review (i.e. there's no need to record an ACK somewhere), but contributors are expected to follow the above guideline.
 +
 
 +
See also [[Gerrit vs GitHub Pull Request]].
 +
 
 +
= GitHub - Initial setup of a remote forked repo =
 +
 
 +
* Go to https://github.com/dogtagpki/pki.
  
* Create a fork of the project by clicking on the fork button:
+
* Create a fork of the upstream project by clicking on the fork button:
  
 
[[File:Gh_pr_fork.png]]
 
[[File:Gh_pr_fork.png]]
  
 +
* Now, you'll have your own copy of a remote forked repo in the format: https://github.com/<your Github ID>/pki.
  
* Now, you'll have your own copy of Forked repo in the format: https://github.com/<your Github ID>/pki
+
= Local Repo - Initial setup =
  
* Now add this remote to your cloned repo
+
* Clone the upstream project into a local repository:
 +
 +
$ git clone https://github.com/dogtagpki/pki.git
 +
 
 +
* Add the remote forked repo to your local repo:
  
 
  $ cd pki
 
  $ cd pki
 +
 +
# If you want to type in Github username/pass for every push
 
  $ git remote add personal https://github.com/<your Github ID>/pki
 
  $ git remote add personal https://github.com/<your Github ID>/pki
 +
 +
# OR
 +
 +
# If you want to use an SSH authentication using your synced key-pair
 +
# Note: This doesn't prompt for Username/password for pushing the changes
 +
$ git remote add personal git@github.com:<your Github ID>/pki.git
 +
 +
If you want to set up SSH keys for github account, please follow [https://help.github.com/articles/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent/ this] guide
  
 
* Check whether you see the following output:
 
* Check whether you see the following output:
Line 29: Line 61:
 
  personal https://github.com/SilleBille/pki.git (push)
 
  personal https://github.com/SilleBille/pki.git (push)
  
= Travis - Enabling Travis CI for forked repo =
+
= Local Repo - Submitting patches from a local dev branch =
  
1. Go to https://travis-ci.org/ and sign in using Github (Top right button)
+
* Within your local repo, checkout the branch to which you want to submit one or more patches:
  
2. Add the forked repository by clicking the '''+''' sign next to '''My Repositories'''.
+
# To submit patches to master:
 +
$ git checkout master
  
[[File:travis_2.png]]
+
# To submit patches to DOGTAG_10_5_BRANCH (skip next step since you are creating a branch directly):
 +
$ git checkout -b ticket-<number> origin/DOGTAG_10_5_BRANCH
  
3. Select the forked repository.
+
* Create a new local dev branch (which you'll be submitting as the PR):
  
[[File:travis_4.png]]
+
$ git checkout -b ticket-<number>
 
 
If necessary, click on '''Sync projects from GitHub''' or '''Sync account''' on the top right of the page to refresh the list of repositories.
 
 
 
= GitHub - Verify Travis CI for forked repo =
 
  
In your forked project, (e. g. - https://github.com/<your Github ID>/pki/settings/installations), go to '''Settings''' -> '''Integration & services''', verify that '''Travis CI''' appears under '''Services'''.
+
* Make changes (or cherry-pick changes from other branches) and commit them to your local dev branch:
  
[[File:travis_5.png|500px]]
+
$ git commit -a
  
= Submitting patches from a local dev branch =
+
* Optionally verify that the commit(s) have been committed to the local dev branch:
  
* Checkout the branch to which you want to submit patch:
+
  # For master:
 
+
  $ git branch
# To submit patches to master:
+
    master
$ git checkout master
+
  * ticket-<number>
 
    
 
    
# To submit patches to DOGTAG_10_5_BRANCH:
+
  # For DOGTAG_10_5_BRANCH:
$ git checkout DOGTAG_10_5_BRANCH
+
  $ git branch
 
+
    DOGTAG_10_5_BRANCH
* Create a new local dev branch (which you'll be submitting as the PR):
+
    master
 +
  * ticket-<number>
 +
 
 +
  $ git log
  
$ git checkout -b ticket-<number>
+
* Push the local dev branch to your remote forked repo:
 
 
* Make changes (or cherry-pick changes from other branches) and commit them to your new local dev branch:
 
 
 
$ git commit -a <changes>
 
 
 
* Push the new local dev branch to your repo
 
  
 
  # Note that ticket-<number> is the new local dev branch you created
 
  # Note that ticket-<number> is the new local dev branch you created
 
  $ git push personal ticket-<number>  
 
  $ git push personal ticket-<number>  
  
* This will trigger a travis CI build. Check to see whether you are ready to submit a PR
+
* This will trigger a travis CI build against your forked repo.
  
 
= GitHub - Creating a Pull Request =
 
= GitHub - Creating a Pull Request =
  
* Submit a pull request from your branch
+
* Go to https://github.com/<your Github ID>/pki.
 +
 
 +
* Submit a pull request from your branch:
 +
 
 
[[File: Gh pr newPR.png]]
 
[[File: Gh pr newPR.png]]
  
* Choose the correct to and from branch. You should see the list of commits that will form as patch. You'll see a screen similar to this:
+
* Go to https://github.com/<your Github ID>/pki/compare (a redirect should occur).
 +
 
 +
* Choose the correct to and from branch. You should see the list of commits that will form a patch. You'll see a screen similar to this:
  
 
[[File: Gh pr correct.png]]
 
[[File: Gh pr correct.png]]
Line 87: Line 119:
 
[[File: Gh pr incorrect.png]]
 
[[File: Gh pr incorrect.png]]
  
* Fill in the '''Commit Message''' and '''Commit Description''' that will appear in the official repo commit history
+
* Fill in the '''Commit Message''' and '''Commit Description''' that will eventually appear in the official upstream repo commit history.
* Click on "Create Pull Request"
 
  
= Merge and update fork =
+
* Add the reviewers (Github provides reviewer suggestions based on the history of the file)
  
* Once your patch goes through and changes are reviewed, your patch is ready to be merged
+
* Click on "Create Pull Request".
* Click on the drop down next to "Merge"
+
 
 +
= GitHub - Merge and update fork =
 +
 
 +
<font color=red>'''NOTE:''' The following instructions are for users with write access to dogtagpki/* repositories. </font>
 +
 
 +
* Once your patches go through and changes are reviewed, your patches are ready to be merged.
 +
 
 +
* Click on the drop down next to "Merge":
 
[[File:Gh pr merge.png]]
 
[[File:Gh pr merge.png]]
 +
  
 
* Use the following guideline to keep the official repo commit history CLEAN:
 
* Use the following guideline to keep the official repo commit history CLEAN:
  
Create Merge Commit = Creates a new commit on top of the last commit in the official branch (Not recommended)
 
 
  Squash & Merge      = Use when you have lot of insignificant commits (like correcting changes from reviewers). This will Squash all your commits into 1 single commit
 
  Squash & Merge      = Use when you have lot of insignificant commits (like correcting changes from reviewers). This will Squash all your commits into 1 single commit
  Rebase & Merge      = Use when your patch is actually a combination of different patches (every commit is significant). This will copy all the commits on top of the last commit in the branch
+
  Rebase & Merge      = Use when your patch is actually a combination of different patches (every commit is significant). This will copy ALL the commits on top of the last commit in the branch
 +
Note: '''Create Merge Commit''' has been disabled to avoid undesirable loss of commit messages
  
 
* Once merged, Github will provide you an option to delete your branch. DELETE IT!
 
* Once merged, Github will provide you an option to delete your branch. DELETE IT!
* Now, to keep your fork clean and in-sync with official repo do this in your local:
+
 
 +
* Now, to keep your fork clean and in-sync with official repo do this in your local repository:
 
  $ git checkout master          # or the branch you want to sync
 
  $ git checkout master          # or the branch you want to sync
 
  $ git pull origin master        # or the branch you want to sync
 
  $ git pull origin master        # or the branch you want to sync
Line 117: Line 157:
 
* [https://help.github.com/categories/collaborating-with-issues-and-pull-requests/ Collaborating with issues and pull requests]
 
* [https://help.github.com/categories/collaborating-with-issues-and-pull-requests/ Collaborating with issues and pull requests]
 
* [https://www.youtube.com/watch?v=FQsBmnZvBdc Git & GitHub: Pull requests]
 
* [https://www.youtube.com/watch?v=FQsBmnZvBdc Git & GitHub: Pull requests]
 +
* [https://makandracards.com/makandra/14323-git-how-to-check-out-branches-that-exist-on-multiple-remotes Checking out branch that has multiple remotes]

Latest revision as of 21:27, 4 September 2020

Overview

Patch review is a way to ensure the patches merged into the source repository are correct and good (i.e. clean, good design, conforming to standards). However, doing a patch review sometimes takes a significant amount of effort, which can take away the time to do other things. As a compromise, the review will be done on important or high-risk patches only.

Here are some situations where a patch review is required or highly recommended:

  • Patches submitted by external contributors have to be reviewed and merged by a team member.
  • Patches for areas that are critical or complex, e.g. crypto changes, replication, serial number allocation.
  • Patches for maintenance/stable branch (i.e. anything other than master branch), e.g. 10.5 branch.
  • Patches for unfamiliar areas (i.e. mainly written by someone else).
  • Patches that will affect test automation in a significant way.
  • Patches that were already reviewed but change significantly when cherry-picked into another branch.

Here are some situations where a patch review is not really necessary:

  • Patches that do not change significantly when cherry-picked into another branch.
  • Patches that are simple and low-risk, e.g. typos, log message changes. High-risk patches should still be reviewed even though it's trivial, e.g. crypto changes.
  • Patches that refactor/reorganize the code without changing the functionality. These patches should be done in simple steps so others can understand easily. Do not make major/complex changes in a single patch.

There are various ways to do patch reviews, e.g. in person, over IRC, via email, in the ticket, but the recommended way is with GitHub Pull Request. GitHub PR allows patch reviews to be done conveniently online. All PRs can be found in one place. The patches are displayed in an easy-to-read format. Anybody can provide a comment on the patch in general or on a specific code in the patch. The CI will run automatically. And finally, the patches can be rebased and merged with a single click.

There is no record keeping required for patch review (i.e. there's no need to record an ACK somewhere), but contributors are expected to follow the above guideline.

See also Gerrit vs GitHub Pull Request.

GitHub - Initial setup of a remote forked repo

  • Create a fork of the upstream project by clicking on the fork button:

Gh pr fork.png

  • Now, you'll have your own copy of a remote forked repo in the format: https://github.com/<your Github ID>/pki.

Local Repo - Initial setup

  • Clone the upstream project into a local repository:
$ git clone https://github.com/dogtagpki/pki.git 
  • Add the remote forked repo to your local repo:
$ cd pki

# If you want to type in Github username/pass for every push
$ git remote add personal https://github.com/<your Github ID>/pki

# OR

# If you want to use an SSH authentication using your synced key-pair
# Note: This doesn't prompt for Username/password for pushing the changes
$ git remote add personal git@github.com:<your Github ID>/pki.git

If you want to set up SSH keys for github account, please follow this guide

  • Check whether you see the following output:
$ git remote -v
origin	https://github.com/dogtagpki/pki (fetch)
origin	https://github.com/dogtagpki/pki (push)
personal	https://github.com/SilleBille/pki.git (fetch)
personal	https://github.com/SilleBille/pki.git (push)

Local Repo - Submitting patches from a local dev branch

  • Within your local repo, checkout the branch to which you want to submit one or more patches:
# To submit patches to master:
$ git checkout master
# To submit patches to DOGTAG_10_5_BRANCH (skip next step since you are creating a branch directly):
$ git checkout -b ticket-<number> origin/DOGTAG_10_5_BRANCH
  • Create a new local dev branch (which you'll be submitting as the PR):
$ git checkout -b ticket-<number>
  • Make changes (or cherry-pick changes from other branches) and commit them to your local dev branch:
$ git commit -a
  • Optionally verify that the commit(s) have been committed to the local dev branch:
 # For master:
 $ git branch
   master
 * ticket-<number>
 
 # For DOGTAG_10_5_BRANCH:
 $ git branch
   DOGTAG_10_5_BRANCH
   master
 * ticket-<number>
  
 $ git log
  • Push the local dev branch to your remote forked repo:
# Note that ticket-<number> is the new local dev branch you created
$ git push personal ticket-<number> 
  • This will trigger a travis CI build against your forked repo.

GitHub - Creating a Pull Request

  • Submit a pull request from your branch:

Gh pr newPR.png

  • Choose the correct to and from branch. You should see the list of commits that will form a patch. You'll see a screen similar to this:

Gh pr correct.png

If there is an error, you'll see something like this:

Gh pr incorrect.png

  • Fill in the Commit Message and Commit Description that will eventually appear in the official upstream repo commit history.
  • Add the reviewers (Github provides reviewer suggestions based on the history of the file)
  • Click on "Create Pull Request".

GitHub - Merge and update fork

NOTE: The following instructions are for users with write access to dogtagpki/* repositories.

  • Once your patches go through and changes are reviewed, your patches are ready to be merged.
  • Click on the drop down next to "Merge":

Gh pr merge.png


  • Use the following guideline to keep the official repo commit history CLEAN:
Squash & Merge      = Use when you have lot of insignificant commits (like correcting changes from reviewers). This will Squash all your commits into 1 single commit
Rebase & Merge      = Use when your patch is actually a combination of different patches (every commit is significant). This will copy ALL the commits on top of the last commit in the branch
Note: Create Merge Commit has been disabled to avoid undesirable loss of commit messages
  • Once merged, Github will provide you an option to delete your branch. DELETE IT!
  • Now, to keep your fork clean and in-sync with official repo do this in your local repository:
$ git checkout master           # or the branch you want to sync
$ git pull origin master        # or the branch you want to sync
$ git push personal master      # or the branch you want to sync
$ git branch -D ticket-<number> # Delete the local branch which you deleted from Github

References