Over a million developers have joined DZone.

Rant about Github Pull-Request Workflow Implementation

DZone's Guide to

Rant about Github Pull-Request Workflow Implementation

· DevOps Zone ·
Free Resource

In response to accelerated release cycles, a new set of testing capabilities is now required to deliver quality at speed. This is why there is a shake-up in the testing tools landscape—and a new leader has emerged in the just released Gartner Magic Quadrant for Software Test Automation.

One of my recent innocent tweet about Gerrit vs Github triggered much more reponses and debate that I expected it to. I realize that it might be worth explaining a bit what I meant, in a text longer than 140 characters.

The problems with Github pull-requests

I always looked at Github from a distant eye, mainly because I always disliked their pull-request handling, and saw no value in the social hype it brings. Why?

One click away isn't one click effort

The pull-request system looks like an incredible easy way to contribute to any project hosted on Github. You're a click away to send your contribution to any software. But the problem is that any worthy contribution isn't an effort of a single click.

Doing any proper and useful contribution to a software is never done right the first time. There's a dance you will have to play. A slowly rhythmed back and forth between you and the software maintainer or team. You'll have to dance it until your contribution is correct and can be merged.

But as a software maintainer, not everybody is going to follow you on this choregraphy, and you'll end up with pull-request you'll never get finished unless you wrap things up yourself. So the gain in pull-requests here, isn't really bigger than a good bug report in most cases.

This is where the social argument of Github isn't anymore. As soon as you're talking about projects bigger than a color theme for your favorite text editor, this feature is overrated.

Contribution rework

If you're lucky enough, your contributor will play along and follow you on this pull-request review process. You'll make suggestions, he will listen and will modify his pull-request to follow your advice.

At this point, there's two technics he can use to please you.

Technic #1: the Topping

Github's pull-requests invite you to send an entire branch, eclipsing the fact that it is composed of several commits. The problem is that a lot of one-click-away contributors do not masterize Git and/or do not make efforts to build a logical patchset, and nothing warns them that their branch history is wrong. So they tend to change stuff around, commit, make a mistake, commit, fix this mistake, commit, etc. This kind of branch is composed of the whole brain's construction process of your contributor, and is a real pain to review. To the point I quite often give up.

A typical case: 3 commits to build a 4 lines long file.

Without Github, the old method that all software used, and that many software still use (e.g. Linux), is to send a patch set over e-mail (or any other medium like Gerrit). This method has one positive effect, that it forces the contributor to acknowledge the list of commits he is going to publish. So, if the contributor he has fixup commits in his history, they are going to be seen as first class citizen. And nobody is going to want to see that, neither your contributor, nor the software maintainers. Therefore, such a system tend to push contributors to write atomic, logical and self-contained patchset that can be more easily reviewed.

Technic #2: the History Rewriter

This is actually the good way to build a working and logical patchset using Git. Rewriting history and amending problematic patches using the famous git rebase --interactive trick.

The problem is that if your contributor does this and then repush the branch composing your pull-request to Github, you will both lose the previous review done, each time. There's no history on the different versions of the branch that has been pushed.

In the old alternative system like e-mail, no information is lost when reworked patches are resent, obviously. This is far better because it eases the following of the iterative discussions that the patch triggered.

Of course, it would be possible for Github to enhance this and fix it, but currently it doesn't handle this use case correctly..

Exercise for the doubtful readers: good luck finding all revisions of my patch in the pull-request #157 of Hy.

A quick look at OpenStack workflow

It's not a secret for anyone that I've been contributing to OpenStack as a daily routine for the last 18 months. The more I contribute, the more I like the contribution workflow and process. It's already well and longly described on the wiki, so I'll summarize here my view and what I like about it.


To send a contribution to any OpenStack project, you need to pass via Gerrit. This is way simpler than doing a pull-request on Github actually, all you have to do is do your commit(s), and type git review. That's it. Your patch will be pushed to Gerrit and available for review.

Gerrit allows other developers to review your patch, add comments anywhere on it, and score your patch up or down. You can build any rule you want for the score needed for a patch to be merged; OpenStack requires one positive scoring from two core developers before the patch is merged.

Until a patch is validated, it can be reworked and amended locally using Git, and then resent using git review again. That simple. The historic and the different version of the patches are available, with the whole comments. Gerrit doesn't lose any historic information on your workflow.

Finally, you'll notice that this is actually the same kind of workflow projects use when they work by patch sent over e-mail. Gerrit just build a single place to regroup and keep track of patchsets, which is really handy. It's also much easier for people to actually send patch using a command line tool than their MUA or git send-email.

Gate testing

Testing is mandatory for any patch sent to OpenStack. Unit tests and functionnals test are run for each version of each patch of the patchset sent. And until your patch passes all tests, it will be impossible to merge it. Yes, this implies that all patches in a patchset must be working commits and can be merged on their own, without the entire patchset going in! With such a restricution, it's impossible to have "fixup commits" merged in your project and pollute the history and the testability of the project.

Once your patch is validated by core developers, the system checks that there is not any merge conflicts. If there's not, tests are re-run, since the branch you are pushing to might have changed, and if everything's fine, the patch is merged.

This is an uncredible force for the quality of the project. This implies that no broken patchset can ever sneak in, and that the project pass always all tests.

Conclusion: accessibility vs code review

In the end, I think that one of the key of any development process, which is code review, is not well covered by Github pull-request system. It is, along with history integrity, damaged by the goal of making contributions easier.

Choosing between these features is probably a trade-off that each project should do carefully, considering what are its core goals and the quality of code it want to reach.

I tend to find that OpenStack found one of the best trade-off available using Gerrit and plugging testing automation via Jenkins on it, and I would probably recommend it for any project taking seriously code reviews and testing.

Overcoming Test Automation Roadblocks with Service Virtualization


Published at DZone with permission of

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}



The problems with Github pull-requests



I always looked at Github from a distant eye, mainly because I always disliked their pull-request handling, and saw no value in the social hype it brings. Why?


One click away isn't one click effort


The pull-request system looks like an incredible easy way to contribute to any project hosted on...","wordCount":1339,"deleted":false,"likeStatus":{"liked":false,"score":0,"canLike":false},"zonetop":"","articleTags":[],"header":{"id":492106,"title":"Rant about Github Pull-Request Workflow Implementation ","imageUrl":"/themes/dz20/images/ArticleImg_6.jpg","link":"/articles/rant-about-github-pull-request","imageLink":"/themes/dz20/images/ArticleImg_10.jpg","titleEll":"Rant about Github Pull-Request Workflow...","type":"article"},"url":"/articles/rant-about-github-pull-request","isLocked":false,"draft":false,"articleContent":"","source":"http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation","modDate":1368233100000,"views":4613,"isLimited":false,"tldr":null,"originalSource":"http://julien.danjou.info/blog/2013/rant-about-github-pull-request-workflow-implementation","articleType":"none"}]; WMODEL_DATA.perms = {"canDecidePick":false,"canPublish":false}; WMODEL_DATA.partners = {"6":[{"details":{"logo":"//dz2cdn2.dzone.com/storage/partner-logo/785001-sonatype-nexus-logo.png","level":2,"name":"Sonatype","partnerUrl":"http://www.sonatype.com/nexus/solution-overview","leveldesc":"Platinum","code":"sonatype"},"bottom":{"id":222230,"text":"

The DevOps Zone is brought to you in partnership with Sonatype Nexus.  See how the Nexus platform infuses precise open source component intelligence into the DevOps pipeline early, everywhere, and at scale. Read how in this ebook


The Nexus Suite is uniquely architected for a DevOps native world and creates value early in the development pipeline, provides precise contextual controls at every phase, and accelerates DevOps innovation with automation you can trust. Read how in this ebook.


Download the ‘Practical Blueprint to Continuous Delivery’ to learn how Automic Release Automation can help you begin or continue your company’s digital transformation.


Download the blueprint that can take a company of any maturity level all the way up to enterprise-scale continuous delivery using a combination of Automic Release Automation, Automic’s 20+ years of business automation experience, and the proven tools and practices the company is already leveraging.


Find out more about how Scalyr built a proprietary database that does not use text indexing for their log management tool.


Learn more about how CareerBuilder was able to resolve customer issues 5x faster by using Scalyr, the fastest log management tool on the market. 


Learn how to measure the impact of every feature release on performance and customer experience metrics.


Planning to extract out a few microservices from your monolith? Read this free guide to learn the best practice before you get started.


Overcoming Test Automation Roadblocks with Service Virtualization


In response to accelerated release cycles, a new set of testing capabilities is now required to deliver quality at speed. This is why there is a shake-up in the testing tools landscape—and a new leader has emerged in the just released Gartner Magic Quadrant for Software Test Automation.


Discover how to optimize your DevOps workflows with our on-demand QA solution, brought to you in partnership with Rainforest QA.


Best practices for getting to continuous deployment faster and with dramatic results in reduced outage minutes, development costs, and QA testing cycles. Brought to you by Rainforest QA.


We did the research and have the tutorials on how to install the top CI/CD tools. See how Jenkins, Gitlab CI, Buildbot, Drone and Concourse compare.


How to Set Up Continuous Integration Pipelines with Drone on Ubuntu 16.04. Get the DigitalOcean detailed tutorial.

"}}],"lastUsed":4}; WMODEL_DATA.authenticated = false; WMODEL_DATA.daysOldBody = "Technology moves quickly and this #type was published #time. Some or all of its contents may be outdated."; WMODEL_DATA.firstArticleContent = null; WMODEL_DATA.isPreview = false; WMODEL_DATA.OPTIONS = {}; TH.installWidgetController('article.content', 'articleContent5', WMODEL_DATA, typeof controller == 'function' ? controller : null, [{name: 'partners', data: true},{name: 'DEFAULT', data: true}], ' oUhbblYOaqbcblYOaqbcC', null); })(); (function() { function controller($scope, $service, $location, SideBarService, $timeout) { if ($scope.edition) { $scope.date = moment($scope.editionDate).utc().format('MMM DD, YYYY'); } SideBarService.ctx.pageSize = $scope.pageSize; SideBarService.ctx.isPreview = $scope.isPreview; SideBarService.ctx.mode = $scope.mode; SideBarService.fn.loader = $service; var $window = $(window); function checkWidth() { var windowsize = $window.width(); $scope.width = windowsize; } // Execute on load checkWidth() // Bind event listener $(window).resize(checkWidth); if ($scope.edition) { SideBarService.ctx.edition = $scope.edition; } SideBarService.fn.scrollCheck = function() { $scope.$emit('thIfScrollCheck'); }; var currentFilter; $scope.$on('$locationChangeSuccess', function() { if (!$location.search().filter) { $scope.filter = 'latest'; } else { $scope.filter = $location.search().filter; if ($scope.filter == 'latest') { $location.search('filter', null); } } if (currentFilter == $scope.filter) { return; } currentFilter = $scope.filter; SideBarService.ctx.filter = $scope.filter; }); $scope.display = SideBarService.getList(); $scope.$watchCollection(function() { return SideBarService.getList(); }, function (n) { $scope.display = n; }); $scope.isActive = SideBarService.isActive; $scope.isExcluded = SideBarService.isExcluded; $scope.loadMore = SideBarService.load; $scope.loading = function() { return SideBarService.ctx.loading; }; TH.on('TapBarStatusChange', function(expanded) { if (expanded) { SideBarService.unblock(); } }) } var WMODEL_DATA = {}; WMODEL_DATA.edition = null; WMODEL_DATA.editionName = ""; WMODEL_DATA.pageSize = 20; WMODEL_DATA.isPreview = false; WMODEL_DATA.editionDate = null; WMODEL_DATA.OPTIONS = {}; WMODEL_DATA.mode = null; TH.installWidgetController('sidebar.content.list', 'sidebarContentList8', WMODEL_DATA, typeof controller == 'function' ? controller : null, [{name: 'DEFAULT', data: true}], ' oUhbkSMaaqbcdvVkcC', null); })(); (function() { function controller($scope) { var $window = $(window); function checkWidth() { var windowsize = $window.width(); var $element = $('div.sidebar.sidebarTapBar'); $scope.width = windowsize; if(windowsize <= 1024 && $scope.edition){ $('.fixContentRight').removeClass('fixContentRight'); // $element.removeClass('expanded'); // $element.addClass('tapNotExpanded'); $('.tap').show(); }else if($scope.edition){ $('.tap').hide(); $('.mainContentRow').addClass('fixContentRight'); $element.removeClass('tapNotExpanded'); $element.addClass('expanded'); } } // Execute on load checkWidth(); // Bind event listener $(window).resize(checkWidth); } var WMODEL_DATA = {}; WMODEL_DATA.edition = null; WMODEL_DATA.slot = null; WMODEL_DATA.OPTIONS = {}; TH.installWidgetController('sidebar.tapBar', 'sidebar', WMODEL_DATA, typeof controller == 'function' ? controller : null, null, ' oUhbkSMadabfWVcC oUhbkSMadabbWQbVkcC', null); })(); (function() { function controller($scope, shareThis, TH$Dialog, TH$Service, $location) { $scope.getEditUrl = function(id, type) { if (!type || type == 'article') { return '/content/' + id + '/edit.html'; } else { return '/dzone/staff/' + type + (type == 'refcard' ? 'z' : 's') + '/' + id + '/edit.html'; } }; $scope.share = function(socialNet, url, title){ shareThis.shareThis(socialNet, url, title); }; // $scope.mailShareLink = function(article) { // return 'mailto:?subject=' + encodeURIComponent(article.header.title) + '&body=Article: ' + encodeURIComponent('https://dzone.com/' + article.header.link); // }; $scope.canDelete = function(article) { return article.canDelete; }; $scope.canPublish = function(article) { return article.canPublish; }; $scope.canEdit = function(article) { return article.canEdit; }; $scope.toggleComments = function(article) { if(!article.isLocked) { TH$Service.action('articles.lockNode', {type: 'node', id: article.id}).then(function(result) { if(result){ article.isLocked = true; TH$Dialog.success('You have disabled all comments for this Article'); }else{ TH$Dialog.error('error','Your requested was denied') } }); }else { TH$Service.action('articles.unlockNode', {type: 'node', id: article.id}).then(function(result) { if(result){ article.isLocked = false; TH$Dialog.success('You have enabled all comments for this Article'); }else{ TH$Dialog.error('error','Your requested was denied') } }); } }; $scope.toggleLimitComments = function (article) { if (!article.isLimited) { TH$Service.action('articles.limitNode', {type: 'node', id: article.id}).then(function (result) { if (result) { article.isLimited = true; TH$Dialog.success('You have limited comments for this Article. Now all comments will go through moderation.'); } else { TH$Dialog.error('error', 'Your requested was denied') } }); } else { TH$Service.action('articles.unlimitNode', {type: 'node', id: article.id}).then(function (result) { if (result) { article.isLimited = false; TH$Dialog.success('You removed the limits for comments on this Article'); } else { TH$Dialog.error('error', 'Your requested was denied') } }); } }; $scope.shareTwitter = function($event, title, url){ $event.preventDefault(); $event.stopPropagation(); var twitter = 'https://twitter.com/intent/tweet'; var link = $location.protocol() + '://' + location.host + url; var ref = location.host; var params = '?text='+title+'&url='+link+'&ref=dzone.com&via=DZone'; var win = window.open(twitter+params, '_blank'); win.focus(); }; $scope.edit = function(link) { TH$Dialog.open({ loadWidget: 'links.postPreview', widgetArgs: { edit: link.id }, size: 'xbig' }).then(function (result) { $scope.link.title = result.title; $scope.link.linkDescription = result.content; $scope.link.thumb = result.thumb; $scope.link.tags = result.topics; }); }; $scope.deleteLink = function(article) { var title = article.title; var type = 'link'; if(article.header){ title = (article.header.type == 'article') ? article.header.title : article.title; type = (article.header.type == 'article') ? 'article' : 'link'; } TH$Dialog.confirm('Do you want to delete "' + title + '"?').then(function() { return TH$Service.action('delete', {type: type, id: article.id}); }).then(function() { article.deleted = true; }); }; } var WMODEL_DATA = {}; WMODEL_DATA.OPTIONS = {}; TH.installWidgetController('content.commentsSlider', 'contentCommentsSlider7', WMODEL_DATA, typeof controller == 'function' ? controller : null, null, ' oUhbaqbcaibvnWffWVcC', null); })(); (function() { var WMODEL_DATA = {}; WMODEL_DATA.name = "commentsSlider"; WMODEL_DATA.slot = null; WMODEL_DATA.OPTIONS = {"name":"commentsSlider"}; TH.installWidgetController('components.slider', 'componentsSlider6', WMODEL_DATA, typeof controller == 'function' ? controller : null, null, ' oUhballbvbdSaoUhM', null); })();