I’ve Found Something BETTER Than Pull Requests...

I think that feature branches and pull requests have their place but for most commercial team-based development they're the wrong choice feature branches hide information any branch by definition does that and pull requests to slow the whole process because they add delays into the development process while we're waiting for someone to review the changes and they add process gates to validate work before it's allowed to proceed the data says that process Gates like these are negatively correlated with quality think about that for a moment let me say it again if you do these things that are meant to make things better meant to improve quality and safety statistically they make things worse so what can we do instead how can we make change quickly and safely without building Gates into our process that slow things down and result in lower quality overall foreign hi I'm Dave Farley of continuous delivery welcome to my channel if you haven't been here before please do hit subscribe and if you enjoy the content today hit like as well sometimes people say that I'm dogmatic and while I can recognize that I'm certainly opinionated about things I don't think that I am dogmatic let me explain I don't believe in or follow any Dogma Dogma is defined as a principle or set of principles laid down by an authority as incontrovertibly true I'm argumentative I hope in a respectful and nice way but I am argumentative I don't take anyone's opinion as correct only because of who they are including my own so I'm not dogmatic I like to form my opinions based fundamentally on two things data and evidence and reasoning based on my understanding I like to have a mental model for why and how things work the way that they do and to see if that fits into with the facts as far as I can understand them and perceive them for example I can explain why I think that continuous integration is so deeply important to doing great working software and that is certainly reinforced by my personal experience but it isn't only based on that let me give you an example I've been practicing continuous integration for years and I value it very highly indeed but even if that wasn't true I think that there is a much stronger argument than only my personal experience what continuous integration does is give us feedback at the earliest possible point on exactly the code that will be released into production we're not testing some close approximation of what may end up in production hidden on a branch somewhere continuous integration is about evaluating everybody's changes together at least once per day that's part of the definition in continuous integration we're evaluating the reality of the current truth of the system this provides a definitive statement to the level of our testing at least on whether our change is good or not nothing else can do that that's it for me that's the killer argument for continuous integration everything else is just guessing crossing our fingers and hoping that my changes don't break yours or vice versa let me pause for a moment and say thank you to our sponsors we're fortunate to be sponsored by equal experts tricentis transfig sleuth and Ice panel all of these companies offer products and services that are very well aligned with the topics that we discuss on this channel every week so if you're looking for excellence in continuous delivery and software engineering click on the links in the description below to check them out of course we can still screw up with continuous integration but there are simply fewer ways for us to screw up my real Focus for today though is code review rather than continuous integration though inevitably continuous integration plays a significant role in this conversation the company's pushback that I get when I talk about continuous integration perhaps trunk-based development versus feature branches and pull requests for example is something like ah so you don't agree with code review or how can you possibly do code review in trunk-based development I value high quality code very highly I think it's possible that I probably think it's more important than most developers do so of course I value code reviews but I want great code reviews not Anodyne bureaucratic box ticking exercises so my preference has always been the continuous in-depth code review of pair programming pair programming has lots of benefits and not just in terms of code reviews but for code review they're more in-depth happen at the time when the review is of the most value when decisions are being made rather than as some form of after the fact semi-academic criticism and they don't impose any feedback in pairing delays feedback is always quick and timely in continuous integration and pairing so pairing has been my usual answer to the question about what about code reviews for years now one of my friends has recently written about an alternative approach though and I thought that I'd bring this to your attention I should probably caveat this with the truth that I haven't tried this exact approach myself on a team so I can't recommend this from personal experience except to say that this does make an awful lot of logical sense to me and fits into my mental model of what matters and what doesn't here I'll certainly be trying this the next time I have the need the idea is to replace pull requests with a process called non-blocking code reviews so let me attempt to lay out the criteria the model that I'd use to judge the value of this idea and then let's try evaluating pull requests and non-blocking reviews against that model to demonstrate this style of thinking perhaps to see how they stand up to each other the data says that continuous integration matters if we want to achieve high quality code efficiently then we must evaluate changes on a frequent basis we must make progress as a series of small steps gather feedback and ideally learn something new from each small step we need great feedback on the validity of our changes so that everyone can clearly see where we stand after each small change this requires some adaption to the ways that we work to achieve it but this is the starting point we'll optimize for fast clear feedback on our changes in continuous delivery the feedback that we're looking for is really the answer to a single question is this change releasable and we want a definitive yes or no answer multiple times a day to that question but at the very minimum once per day if you can do that you're practicing continuous delivery and if you can't then I'm sorry but you're not there yet it really is as simple as that this may be easy or it may be very difficult for more complex systems and bigger groups of people this may demand that you change a lot of things for smaller simpler systems and very small groups of people it'll be easier but the rules are still the same parts of the game in achieving this level of feedback is to optimize things and certainly at the more complicated end of the spectrum that means optimize pretty much everything this is one of the reasons that I describe continuous delivery as a lean process our aim is to do the maximum amount of work but with the least amount of effort so we need to make things as efficient as we possibly can everywhere really certainly in complex systems so these ideas are my starting point for creating an effective development process we want fast clear feedback feedback in minutes and not days we want to be able to determine the release ability a go no go decision at least once per day for our system we want to expend the minimum amount of effort make definitive progress each each thing that you do you want to do only once and only once and we want to be skeptical about any pauses or Gates that exist in the process do these things really add value and does that value really out the way the cost of the delay these principles are useful in steering our choices for how we decide to organize our work the data though is pretty clear the idea of slowing down for safety is a mistake we don't improve quality by adding more bureaucracy to the process this is counter-intuitive but fundamentally you can't inspect quality into a process you have to organize things so that you can build it in and that means shortening the feedback cycle so you can see that you have built it in which gets me to the problem of code reviews I think that this is probably true that the most common form of organizing code reviews doesn't follow my advice it's based on pull requests I've talked about this before my usual advice has been to recommend pair or mob programming these are better quality code reviews they're constant and pervasive code reviews and they don't add any pauses delays or approval gates to the process for a long time that was it that was all the advice that I had on this topic really I still think that this is good advice and that despite the very common skepticism about pair programming it tends to be better liked once people have tried it than the people expect before they've tried it to be honest that includes me I didn't imagine liking peer programming before I'd ever tried it but once I did I did like it however I do accept that not everyone does and that pairing is a very big cultural stretch for some teams and organizations I think that they're making a big mistake they're throwing a huge amount of value out when they reject it but it is their choice in these circumstances I've tried a variety of different approaches but they were not a real replacement for the code review part of pair programming and came nowhere near the other benefits then my friend Thierry told me about an alternative that had worked for them this makes sense to me and to my mind is the combination really of two ideas the first is simply of arranging for a rolling process of review a set of regular reviews as part of the development flow thierry's team added an extra step to their process represented as an extra column on their kanban board meaning this feature is ready for review I've seen this done before but every time I have seen it it was usually treated as some sort of gate in the process this creates all sorts of problems let's just think of this in the context of continuous delivery for a moment my preferred definition of continuous delivery is working so that a system is always in a releasable state so we optimize for that we create deployment pipelines that evaluate our changes quickly and definitively releaseable or not after each commit with pair programming obviously we can review the code as it's being written but if we're not reviewing the code as it's been written when are we reviewing it we could hold off the review and wait until we have enough code to be worth someone's time to review it this delays continuous integration and that measurably compromises the efficiency of the development process and the quality of the code that we put out this is the choice that teams that use feature branches and pull requests make they wait until a feature is finished and so it's ready to be merged into trunk and so now we have to work on waiting on the shelf for somebody to review it and all the time the state of the trunk is potentially diverging further and further from the last time that the stuff on the Shelf was checked and was working so this is an ever increasing chance of there being a problem when we finally get around to merging this change onto trunk there are other problems with this way of organizing reviews that I've talked about before but today let's look at how this process of branches and pull requests stand up to my criteria for a good process well we don't have fast clear feedback feedback in minutes unless we can complete our features in minutes because we don't know that our changes are releasable until they've been merged and checked alongside everybody else's we don't really expend the minimum of effort either because we have the small extra work of creating branches merging changes between them possibly dealing with merge conflicts creating pull requests and dealing with any feedback from those each of these may be a small amount of work not always but usually but it's still more work than we do if we're practicing full-blown continuous integration I suppose that strictly the answer to the question of are we able to make definitive incremental progress to evaluating our changes is it depends most teams that I speak to that practice feature branching tell me that they practice continuous integration by running CI on the feature branch strictly by definition this isn't CI because what's on the feature Branch doesn't contain everybody else's changes presumably those are hidden away on other people's feature branches so now it's impossible to make definitive progress and do each thing only once because at a minimum you'll have to rerun CI when you merge the release onto the branch so that you can then finally evaluate your changes alongside everybody else's we certainly aren't being very skeptical about any pauses or gates in this process either because the whole process is hiding information behind emerge gate and pausing and again gating the process with pull requests does this approach really add value well yeah I think if I'm being honest it probably does but only in a very very narrow sense and with some pretty heavy caveats as to what we mean by value yes it means that developers can work more independently at least for the period of time that their code is hidden away on a feature branch I'm afraid though that I think this is largely an illusionary benefit because all that you're really doing when working this way is deferring the costs in this case until merge time and the data says that this deferral makes you write worse code and more slowly continuous integration works better because it keeps things hidden for much shorter periods of time if you'd like to learn more about my thinking about what a really great process looks like I have a new free training course there's a link in the description to that below finally the killer corollary is to that question of does it add value is is it worth it and the data says no so feature branches and pull requests score pretty poorly here so if we don't do that and we don't want to pair or mob program what else could we try if we commit the changes to trunk or origin main then we have on reviewed code alongside reviewed code if the presence of a review is one of the factors used to determine its release ability then we now have Schrodinger's repo this cat is releasable and not releasable at the same time yeah okay I know that's not really the case but I did like the joke how do we tell those changes apart what does releasable into production mean now if we've got a repository and a version in our system of our system that contains both releasable and non-releasable parts thierry's team had made a simple and with the benefit of hindsight obvious choice stop the review being definitive for release this is the second key idea Thierry has written an excellent blog post that explains their thinking which is linked in the description to this video as I said their starting point was to add a column to the kanban board to list work that needed review when a feature was finished and ready for use it was added to the review column whenever anyone on the team finished a piece of work started a new piece of work or just at the start of each day would they would check the review column and if there was something to review they'd review it there was a rule that somebody should review only code that they hadn't been involved in writing there weren't any rules about the seniority of who reviewed whose code this team was doing true continuous integration via trunk-based development and so was proceeding via a series of small fine-grained commits the kanban and the reviews were working at the level of whole features so the software was growing small commit by small commit as it should in continuous integration but this means that the the unreviewed code could end up in production whether the feature was finished or not this may look like a risky strategy but as Thierry says I didn't see this as a problem unreviewed code doesn't mean that there are bugs in it the automated testing in the deployment pipeline is what protects us from Bugs and is usually a lot smarter at finding them than code reviews bug finding is not the goal of a code review code reviews are really about code quality so the risk of working this way is not that the code will be buggier but rather that we may end up releasing code of lower quality into production this is code that works because the tests said so but that could be better at some level that's always true code review merely reduces the chances of releasing lower quality code and that depends on the quality of the review it doesn't eliminate it altogether but also it's not the end of the world if this happens remember the tests are all passing particularly if we're working in a continuous delivery environment because as soon as the code is reviewed and we identify where the quality could be improved we can make the changes that improve it and then commit them and then whether the code is already in production or not the fixes will be in production after the next release so how does this stack up against our model for a good process The Continuous integration in the deployment pipeline ensures fast clear feedback feedback in minutes so this approach certainly passes that test deployment pipelines are there to determine releasability and if you can't do that at least once per day it doesn't really count as continuous delivery anyway so yes he does that too we could quibble over whether or not we're expending the minimum amount of effort I think and I'm sure Thierry would probably agree that pair programming is a little better because it's a bit less work it also has lots of other team focused benefits that go well beyond only code review but given that the reason that we're doing this in the first place is because pairing has been rejected for some reason then this seems like a minimum work approach in the context that pairing is excluded as an option this approach certainly allows us to make definitive progress building on what we learn because once again The Continuous integration of the deployment Pipeline and the rest of the evaluation in the pipeline does that this approach has certainly eliminated the need for pauses or gates in the process there is no need for spaghepticism here because there are no pauses to be skeptical about that also means that we don't need to worry about the value of these expensive delays because there are no expensive delays I can imagine nuances and differences in this process that might help to tailor it to different circumstances I like that this about this approach with the high functioning team for example you could imagine doing reviews on demand if you wanted early feedback on a particularly gnarly piece of code you could add a card to the review column with a few notes on it asking for someone to pair with you for the review when they came around to reviewing it actually you can also go the whole hug and ask somebody to pair with you on the gnarly piece of work before you started it but now we're back to my pairing pairing bias leaking out again I really like this strategy the only problem that I can see here is not really a problem with the approach it's a problem of the bureaucracy that sometimes surrounds development pair programming works better in these circumstances there are many regulated industries that demand sometimes for good reasons that changes are reviewed in finance or healthcare for example as a pair we can sign off the review in effect at the point when we commit the change by tagging it with the idea of each developer in the pair non-blocking review doesn't allow us to do that so that if the release into production is gated by this need for a review as it is in some regulated Industries this non-blocking approach may not work under those circumstances so for that pair or mud programming is the better solution fundamentally though all of these non-blocking continuous review strategies are significantly better than any of the Alternatives because they allow us to maintain the flow in the process and so provide us with more efficient more definitive and so higher quality processes as a result thank you very much for watching and I hope that you've enjoyed this today if you do enjoy our content here on the continuous delivery Channel please do consider supporting us via our patreon community there are some great advantages to joining patreon and you can join for now for as little as two pounds a month thank you again bye-bye foreign foreign

Channel: Continuous Delivery

Views: 48,442

Keywords: pull requests, what is a pull request, how to do pull requests, code review, code review example, github pull request, github, code reviews at big companies, how to write code reviews, programming, computer science, software engineering, software development, Dave Farley, continuous delivery, pull request, git

Id: WmVe1QrWxYU

Length: 23min 35sec (1415 seconds)

Published: Wed Jun 14 2023