Over the last few months I’ve been serving as a coordinators for the ngOfficeUIFabric project where we are building directives for Angular 1.x for the Office UI Fabric controls. We have adopted some practices and patterns, most borrowed from other projects that have helped us in managing the project. This post is one of a few in a How We Do It series of posts to share some of our guidelines that may help others.
Those contributing to the project are doing so on their own time. Anyone is welcome to contribute and not be saddled with the burden of a lot of rules. My goal has always been to put things in place that make it easy for new people to get involved, to make sure the management of the project and tedious stuff is stuff contributors don’t have to worry about.
Check out the first post in this series: ngOfficeUIFabric - How We Do It - Keeping a Clean Commit Log
In this post I’ll explain what we consider a good pull request and how we deal with merging them. If you’ve worked with any project that leverages pull requests, you will notice we aren’t doing or saying anything all that unique. However how we merge PRs into our source tree may not look familiar.
Review Common PR Process
Before we get too far, let’s review what the typical contribution process looks like. First, contributors fork the repo to create their own copy. Then they create a branch off the
dev branch (the name of the branch does not matter). Contributors then do their work on that branch and when work is complete, they push it to their fork.
Before creating and submitting the PR, you need to make sure your branch has all the commits from the current
dev branch in your branch. This is referred to as making sure you are in sync. Why does this matter? Because if you are not in sync when your PR is submitted, there’s a fairly good chance that your contributions could conflict with the code in
dev. In our case, if there are conflicts, your PR will be kicked back and you will be asked to fix the conflicts.
How do you sync you branch? Easy… first you need to jump over to the
dev branch & make sure it is current with the upstream (the original repo, not your fork… your fork is origin):
Now, jump back over to your branch where you are doing you work (we’ll call that
feature-branch) & rebase it. What does that mean? Think of it like this. Let’s say you originally created the branch form
dev on Monday. It is now Thursday and changes have been going on since Monday. What you are doing when you rebase is effectively saying “act like the start date for
feature-branch was really Thursday.":
You may get conflicts or merge conflicts. If you don’t, hey all good. But maybe you get conflicts. These are good to find before submitting the PR because if you didn’t do this, you’d get the shame of the rest of the contributors for having your PR kicked back because of conflicts found when merging. OK, just kidding… but wouldn’t you want to deal with them first?
Don’t - Single Commit Closes Multiple Issues (Single Responsibility Commits)
I said a similar thing in the last post about commits where each commit should have single responsibility. This is one rule we don’t budge on - a single commit should only close a single issue, not multiple issues. The only exception is if one issue stems from other issues like there’s some cascading regression. But that’s rare in my experience.
If a contributor submits a PR that includes a commit that closes multiple issues, we insist that the commit is broken up into separate commits that address each item. Take a look at this PR: https://github.com/ngOfficeUIFabric/ng-officeuifabric/pull/201.
As you can see from the screenshot it closed three issues as it did three things (add a new directive and refactor two other directives) but there’s only a single commit. This is not what we want… instead what I did was break up the commit, create three separate commits for each action taken and reference the appropriate issues:
Breaking up a commit is actually pretty easy. This is effectively you saying “undo the last commit action, but don’t change the code, just act like the commit never happened”… think of it as a commit is a glass jar containing a bunch of commits. You are breaking the glass jar and left with it’s contents you need to repackage.
If it’s the most recent commit you want to address (which if you follow the process I outline blow in the section “Merge PR’s Manually” it should be because you isolate the PR from other code in it’s own branch) you simply break the most recent thing on your HEAD:
git reset HEAD~
Now you are left with all the files that were in the commit with changes applied. You can see this by running
git status. At this point you selectively.
If you need to go further back in the tree, then you can do something like
git rebase -i HEAD~3 where
3 is how many commits back it is. In the interactive session you find the commit you want to break up and change it from
edit, save & exit. Now the rebase will stop just after the commit you want to edit so now you run
git reset HEAD~. Repeat the process above in committing each file(s) and adding the correct message. When finished, continue with the rebase (
git rebase --continue) which will take you back to your branch (if it doesn’t you have additional cleanup work to do, but following these steps you should not have any).
This last step is a matter of preference… but since you break the commit and create new commits, you will be the author, not the original contributor. I don’t like this so I rewrite history which is something I explained in my last post, making sure the original contributor is listed as the author on the new commits. You can see how this gets manifested in the log from this commit: 98b142a2:
Ideal - One PR Per Issue Closed
Now, it’s ideal to have a single PR address a single issue so that means that it would have a single commit in it. This isn’t a hard and fast rule we enforce… it’s more of a preference.
A PR should at least address related issues… it should not include a bunch of unrelated issues. But if it closes two or three related issues, as long as each issue is tied to a specific commit in the PR, it’s acceptable.
Clear Title & Description with Referenced Closing Issue(s)
Do I really need to explain this? Make it clear what’s going on so the person merging can get in your head and understand the context. A good example of this can be seen in PR210. You can see it closes issue 17, what it’s actually doing and some extra stuff.
I recommend using GitHub’s new support for PR templates too: https://help.github.com/articles/creating-a-pull-request-template-for-your-repository
Merge PR’s Manually
And for the grand finale, how do we merge PR’s? It’s not as simple as pressing that green button in GitHub!!!
Why? Because that generates merge commits… you’ve seen those in my last post… they don’t do a damn bit of good except make a horribly dirty & unusable commit log!
So what do we do to avoid this? Admittedly I lifted this process from the Angular Material team… you’ll see our docs on the process are very similar but I’ll outline it here with some more explanation.
Let me first explain it from a high level, then dig deeper. From a high level, the process works like this:
- create local branch & pull in the PR changes
- run all automated checks (tests, vetting, compilation, etc)
- check demo
- address any commit cleanup work
- merge into the main
Let’s pick through these things one at a time.
The 2nd biggest complaint I have with GitHub’s big enticing green merge button is that all the work happens on your hosted repo. To me, that thing should be treated like a bank ledger as much as possible - you don’t change it. If you click this button, it makes changes to your main repo in GitHub, not locally where you can verify what you’re doing before you make the change. Merging is a big deal and I want to have a chance to say “whoa… what the hell did I just do” and push the eject button (in other words: delete the local clone, re-clone and start over with what’s in the source of truth: GitHub).
See the link in the image above from GitHub that says command line instructions? That gives you the commands you want which is basically the following (let’s assume I’ve submitted a PR #555 from my forked repo in the branch newfeature-issue12. I’d do the following:
At this point I now have a branch with the changes in the PR.
Check Everything Meets the Coding Bars
Now here I want to ensure that before I even look at the actual code, it passes all checks. I want to be as clean as possible when I do this and not rely on any legacy stuff.
Because our build process uses Node.js, we use node modules so I blow those away & re-download everything, as well as our built library & clean out previously compiled TypeScript.
Next up, we build the project. Since we use TypeScript, the transpilation process will find any errors. Our TypeScript setup uses a
tsconfig.json for all settings so we run it using the following:
If there were any errors, it’s a coding error so we kick it back as a “build failed” to the contributor and abort the merge. The contributor should not be sending this if build fails so as far as I’m concerned, this one is up to them to fix.
Next, run the tests:
gulp test --verbose
Again, any errors here get kicked back. Bad contributor for submitting a failing test!
Last but not least, I want to see the demo, so we build the library & open the demo in question:
If everything looks good, I consider this one finished!
Check the Commit(s)
I then check the commits to make sure they follow the patters above and in the other post I already published, including breaking them up, squashing them together & making sure they reference closed & referenced issues.
This last piece is important because when the PR is closed, the issues will be automatically closed if referenced correctly in the issues and PR.
Merge the PR changes into the Dev Branch
Last step is to now push the code into the
dev branch. Because we created this branch from
dev, this part is easy. What we do is simply jump to
dev & rebase it on the PR branch. This will add the commits from the PR to the tip of
dev without the merge commits!
That’s it… push to GitHub and you’re good! I then close the PR and make sure each issue was appropriately closed as well.
Downsides to this Approach
There are two downsides to this manual approach. First GitHub uses the merge commits to indicate the PR was merged in their browser interface. That’s what the purple icon indicates. The red icon indicates the PR was closed. If you use this process I outlined in this post, you’ll only get the red PR close icon. No biggie though… that’s not important.
Second, this might sound like a lot of work? Nah… script it like we do! I have this shell script check-pr.sh that prompts me for the PR number, the contributor’s repo URL and the name of the branch as well as the name of the directive to test. It does all this automatically for me:
Hopefully this gives you some insight and guidance, or maybe you learned something that you can apply to your project. At any rate I wanted a post I could share with contributors to the project so they could see how we did it… hopefully it helps others.
Now onto working on the next post in this series… this one is going to be fun… stay tuned next week!
Page update history / changelogThis page has been updated since it was originally published. The following list details each change and when it occurred.
- Thursday, August 15, 2019 - 8:14 AM
Clean up HTML to markdown, convert code samples to gists.