Get introduced to the concepts, benefits and key practices of Continuous Delivery, and develop your knowledge of the first steps to start building better software faster. This course is IDEAL for people who are new to Continuous Delivery and non-technical people who want to understand more about CD. Study the fundamentals of Continuous Delivery FOR FREE HERE ➡ courses.cd.training/courses/cd-fundamentals
Dave starts explaining the technique at 17:25. My summary: - team must use trunk based development with continuous integration - code is written with small commits and merged to trunk when complete, then the ticket is moved to a 'ready for review' column on the team's kanban board - code is reviewed when someone is between pieces of work at the start of the day - unreviewed code may be released, however the CI process provides confidence that the system will still work
I think this process could work for some but not all. Depending on your product, ci cannot catch everything. There have been many times where my team mates have helped me catch something before I merged /released. Having a phased rollout I think is really important. Use your user base as part of your regression tests instead of trying to ship perfect code.
Step four: unreviewed code is released. Step five: no-one reviews it the next day because they're being stamped on by the business to "deliver" features.
Our code reviews (as pull requests) also find gaps in test coverage due to developer inexperience. So, we can't safely argue that the tests contained in unreviewed code provide us the surety of finding/preventing bugs, because there won't have been any oversight to ensure gaps don't exist in the added tests. I have worked with engineers who are so experienced that you could safely drop code reviews, however that is not the majority of engineers; if you have any team members who aren't in the top tier of experience and ability, then allowing them to commit to master and calling that releasable as far as I can tell, sets you on a path where it is a matter of time until you break production because they release a bug alongside test code which insufficiently covered the bugged code in question.
most companies also run automated quality checks that run as part of PR process automatically. If you pair, you pair is also human it does not catch everything, you can easily forget to run the necessary checks before committing, and git hooks don't always work well with any build tool (works ok with husky / does not work well with gradle for example)
Test coverage could be verified by an automated pipeline which should mark the code as unreleaseable if the coverage is not enough. It would not check the quality of the tests, though, unless you'd use mutation tests but those tend to be slow. I still feel safer if the verification pipelines fail before my code is the main branch, therefore short-lived feature branches and PR's still seem a good option to guarantee that: 1) main branch contains only releaseable code and can be deployed immediately 2) I don't always have to run the whole test suite locally before pushing my changes. If I missed somethiong, I get the feedback from automated pipelines before my changes affect other teammates. I'm aware that it might be the old habits speaking, though :)
I have seen places where they try this. The problem with those “small changes” continually merged into prod is that over time code becomes a pile of patches one on top of another , each managing to hit its target but not taking care of the overall code layout. The better the team, the longer it takes, but eventually everyone justifies these hacky approaches saying that they cannot refactor the whole thing and the thing needs to be rewritten from scratch. But even in top teams, after two years of practicing this, the mess is unsalvageable. Simple problems need simple fixes: if you are afraid of big merges , simply merge your branch with main everyday.
@@Sjwnbszhbsjnbvvs +1 to that! I frequently review my own PRs (whether I'm the sole developer or not) to make sure everything is as I intended to check in. I've caught many potential bugs and accidental file check-ins like this.
Who's writing the automated tests though? If nobody is reviewing the tests you don't know if the tests are satisfactory. If the tests are wrong or missing cases you're immediately in trouble without reviews. You mentioned the data says pull requests are worse than trunk based, but who are the subjects of this data? High level teams with top talent might be able to release without review, but in many companies it won't work unless someone expirenced reviews the code. Pair programming is at least doubling the cost of a dev team. As you already said, companies aren't going to do it. The data isn't strong enough.
I was just on a project at a US Fortune 500 company that is actively doing pair programming. I had never seen it in action before. Now I'm in favor of it. The review process happens concurrently with the development. Yes, it can interrupt 'flow' and it takes some learning. But I was impressed with the results.
"Who's writing the automated tests though?" - There are a number of ways of addressing your concerns, for example, you can have automation setup to flag code without tests and possibly use something like AI to help more junior developers write code. The goal here is to keep code changes small so that it's much harder to miss possible cases. "High level teams with top talent might be able to release without review, but in many companies it won't work unless someone expirenced reviews the code." - Why would this not work in many companies? Do you mean that many companies don't want to change, if that is the case then it's a totally different question. The question I'd ask is have you tried proposing this change? In practice changes like this are difficult to introduce and require a lot more (and I mean a lot more) than one meeting with the stakeholders and may take months or years. "Pair programming is at least doubling the cost of a dev team." - There are various papers that state that you do lose output when pairing, about 10% as far as I remember, but you do gain a substantial amount of quality (and other benefits) as a result. This goes back to introducing changes in the company, which is not easy.
Pair programming is a makeshift and outdated reaction to rapid growth and change in technology. It's an adhoc tactic, avoiding investment in mentoring, training, and distributed engineering practices. My two cents worth :)
I think the code review process solves the wrong problem. Usually we have this gate, because we don't trust people, or we are not sure if they produce good quality code. The better solution is to teach people how to write good code. My team does this through pair or ensemble programming and regular coding dojos for the whole team, where we learn how to break a problem into very small chunks (usually with 5-7 minutes of work). We learn from each other, we learn how to communicate with each other, we share ideas, we discuss the code, the consequences of doing something, the architecture, etc. I think the discussion is one of the best learning tools. The next problem with code review is that you can always say, it's not my fault, those guys checked it and didn't see a problem. There is a lack of responsibility, and I think a lack of trust that less experienced developers will merge something stupid into main. Another thing I don't like is that code review is sometimes harmful. Imagine you have been writing code all day and the other people say you have to rewrite it completely. It's demotivating and cruel. Instead, we should write the good code from the beginning. My team does not always do things in pairs. We've allowed ourselves to push and release code without someone else checking it, and we haven't had a bug in over a year. I trust my colleagues because I know how they work. They use TDD, they write tests first, then code. I know that if anyone has any doubts, they will pair up with someone else. Long story short, you should build in quality into the process and build trust.
This non-blocking review strategy definitely seems interesting to me. I feel tempted to introduce it and use it as a back-door to getting us to actual pair programming. :)
The data about post-hoc inspection has been around for decades: 1) Cost of Delay 2) Additional labour cost 3) Defect rates increase with amount of inspection. So Pull Requests "solve" (ehum) one problem, and thereby institute three new problems. Back in my Six Sigma days, we always used to say that "an ounce of prevention is worth a ton of cure," i.e. if by any means you can improve in-process quality, you can reduce the cost of poor quality by orders of magnitudes. PR's only have a place where you don't have control over in-process quality, i.e. open source projects with unknown contributors. In all other situations, in my opinion, PR's are a sign of QA not doing their job (i.e. figuring out why things go wrong in the first place, and initiating improvement there.)
For me the PR process is not merely to prevent bugs or ensure tests are passing but it's much more than that. I see a quality assurance aspect here that is far more important in terms of ensuring the code follows a common shared theme/style and approach (tools are available but it's more of a shared software team vision). As a Tech Lead I don't want to have to spend future days unravelling and refactoring someone's code that was poorly implemented from an architectural point of view (even if it is bug free). I'd rather shift left and identify any architecture creep early in the PR
Yes! Yes! I worked for a feature phone platform company that put all of its competitors out of business (before being acquired) by using a similar approach. We cut the development time for a feature phone from a year to 3 months. For the most part, we communicated through code - both locally and remotely.
Great video, thank you Dave. Identifying that anything that introduces a gate in the software delivery process carries a cost of blocking a continuous flow and we should think if there are better ways to achieve the outcome. Code reviews definitely fall into that category. Some comments correctly point out that code reviews do catch issues like security holes, incomplete test coverage, missed use cases that will affect quality. I would argue that if you are currently relying on code reviews to catch these, it is already late. They should be caught before the code is written.
Just a thought about merging before code review, while I fully agree that the purpose of the review is to ensure code quality and not to catch bugs there could be other potential issues. In my experience sometimes more junior developers may introduce potential security risks with their code and if those make it to production this can be catastrophic. Security risks often can not be caught by tests or QA, only a pen test will discover it or if you are unlucky an actual attack. I guess junior developers can be assigned as not trusted and as such will not be allowed to merge without a review or have to always do pair programming but this comes with its own challenges. Such a mistake is not also reserved for juniors, anyone can introduce security risks without noticing. I am not trying to bash the idea, just some perspective
Labeling juniors from the start as 'not trustworthy' will significantly damage confidence and motivation. They will have a hard time trusting their own abilities and won't share their thoughts on the project ultimately preventing them from becoming a good developer. This sort of elitist thinking is what causes a lot of problems in software development which I think Dave is trying to prevent with his approaches.
@@mrspecs4430 Yes I agree, this is why I wrote "this comes with its own challenges". segregating the developers like this is very bad for the team as a whole and could hinder their own development. More experienced devs are also not immune from making mistakes as well. My point is that nothing comes without some side effects, second pair of eyes in a code review is quite good for everything else except that it's inherently a bottleneck. When do away with it we increase the the speed of development but we are exposed to more risk, saying that we are not is simply not true. In a sense, like with everything else, there is no one superior model that is the "best" for all teams and companies. I think people should look at the options at hand and the implications and choose what fits them best as opposed to follow blindly some model of working
@@mrspecs4430 On a side note, labeling junior devs as 'not trustworthy' will probably have the effect you are describing for some of them and the exact opposite for others depending on their personality because some people might push harder to achieve the worth status. We can't know for sure just by guessing, as this will be pure arrogance.What I do know is that it will be an environment that I wouldn't want to work in.
@@mrspecs4430 whether you specifically label them as not trustworthy is not as relevant as how you end up treating them. And you will end up treating code from junior developers as less trustworthy, because quite frankly it simply is. It's only correct to make that assumption. That's why they're junior developers. If their code would be trustworthy, they wouldn't be junior devs, they'd be senior devs. The lower trustworthiness of their code is implicit to what it means to be a junior developer.
@@aaronhamburg4428 I agree there is no silver bullet approach but it's important to re-evaluate our common conventions and recognize our own biases from time-to-time. For example, if security is that important to the project wouldn't a more robust solution be to: 1) Document common coding security anti-patterns and risks and make sure new developers are aware of them as part of the onboarding process 2) Require that all developers earn and maintain some kind of certification such as Security+ 3) Use code vulnerability scanning tools to ensure major security risks are identified. You could still choose to have PR's if that works for your team. Code reviews have several benefits. However, the notion that security risks are a "rookie mistake" and the only line of defense is a wise, hawk-eyed senior dev making sure the code stays pristine seems a bit biased to me. That strikes me more as something to ensure a pecking order than to truly benefit code security.
I agree with a lot of the commenters who say that one thing code reviews catches is poor tests or no tests. But also this approach assumes you already have test coverage, which may not be the case, and I think this would be disastrous in that case. In addition, there's the cultural problem that product always wants more and more features and so if you have code that "works," you might have to fight pretty hard to get the opportunity to go back and make it better--even if you caught that it needed improvements right away. Having that approval gate lets developers conceal that process from the business so code quality can be maintained.
So true about code reviews being about code quality and not catching bugs. At my company, we used to do code reviews after code was checked into the mainline trunk. We have since then "upgraded" to using git PRs and now the changes are tested in our CI/CD pipeline much later than before.
It’s mostly to ensure the checked in code is maintainable by others going forward. Cleanup after checkin is unrealistic because there won’t be any time for that later.
@@mecanuktutorials6476 Then fix that, because THAT is the problem. If you realize you're not taking time to improve code quality, then how does imposing PRs and code reviews fix that? You still have developers that cannot be bothered to write code well in the first place. I'm so sick of these excuses. All these processes make for WORSE developers. "Oh I don't have to write this well because X will tell me what I need to change anyway" How about becoming a better dev and writing things readable in the first place? How about taking responsibility for the code and if you see something that's not well maintainable you fix in righ then and there instead of writing someone else a "review" so they have to fix it. All this crap is just lazy bandaids and damages both developer growth and a feeling of teamwork and trust.
@@FudgeYeahLinusLAN these 10x moments usually end in bad code because you end up coding for hours on end. Pair Programming gives you a framework to work on code sustainably and with a clear mind. You switch every 10-15 minutes and take a 5-15min break after you and your pairing partner had their turn. This is in my experience produces way way better code, removes the need for review & is much more sustainable aswell as a more sociable experience which is a plus if you have nice coworkers.
There are two states in which we *should* be operating: 1. Complete trust, so there's no need for pull requests. 2. Building trust, so we should be working very closely together to ensure we're learning each other's strength and weaknesses: i.e. pair programming. If you think about it, pull requests don't make a lot is sense, except for open source.
While at Thoughtworks, we used to have Code reading sessions in my projects. So instead of reviews at PRs, we would go through the diff weekly once, entire team. And let the team think about what can be improved. And if something was worth improving, we would do it during next change. And everyone now knows and agrees to the need to change.
I started my career in programming long (21+ yrs) ago doing programming in pair with a product manager. It was an incredibly inspiring experience that sparked my interest in IT career. This is why I strongly agree with you, Dave, and believe pair programming not only delivers great value but also provides a cool engineering experience.
I don't see how working on the same branch magically avoids merge conflicts. Eventually all the changes need to be merged. Instead, merge conflicts are reduced by keeping each change small, irrespective of whether you use PRs or trunk based development. Since I can't use pair programming (open source, different time zones), what I do is if the PR gets too big I tell the author to split it into multiple PRs, and try to have enough work queued up so they can continue doing something else while waiting for a review. Also I tell people to regularly rebase, not merge, the main branch.
If you split something into multiple PR's that depend on each other and then someone suggests a change in the first one you now have to rebase all the other ones which is annoying and wastes time. Rinse and repeat for each other PR until the last one. And the longer it takes you to merge the first PR the more out of date with trunk all the other ones get. What he advocates for is yes, split the work into small parts, but commit each part to trunk instead of waiting for reviews in a PR (he advocates for pair programming so that each commit is already reviewed anyway).
@@ricardoamendoeira5689 I understand, I tried pair programming and I actually like it and see how it can improve the process, but I can't use it since there is not enough overlap of time availability. I also understand it's better to preempt a PR from growing big instead of telling to split it later. But again I don't always have these options.
Dave, thank you for your great advice! I just want to point out to others that feature-branch based code reviews can still be an improvement to lesser strategies. For us, it was. We have a bunch of developers but also a large code base, so we don't run into merge conflicts very often. Most of the time they are easy to fix. And when we merge, that very rarely breaks anything. While it's not perfect I think our amount and quality of testing is reasonable. One little thing that helps a bit is that the CI/CD platform we use always runs the build and tests on a temporary merge of the feature branch with the trunk. I know, that is also not optimal, but better than not doing it :) Yes, we can improve and we should sooner or later. But we came from a worse place.
I can go with pull requests or pair programming, but I'm not convinced by review after release, where nobody has reviewed the code before it goes out. I think this is more likely to lead to bugs, and as we all know, bugs slow you down in the long run. Higher quality up front is faster. Code review is also a great time to break down knowledge silos, and for junior developers to learn. Also, the tests might pass but they might not be right. Even with TDD, people make mistakes. The tests might test one thing but actually it's not the correct behaviour because of a misunderstanding, or a junior developer might be new to TDD and add more than the minimum code required to make the tests pass etc. There's also performance. Performance tests are great to have but they can be slow to run, giving slower feedback, and it's quite common to catch performance problems in a code review before the performance tests. Where I work we do pull requests with a minimum of 2 reviewers, and the average completion time is about an hour. In that time, you can keep working on the next thing, so you're not really waiting at all, and you don't deviate too far from trunk. We can still deploy multiple times a day, but what we deploy is higher quality as a result. We do this by using a pull request dashboard for visibility, with desktop notifications (e.g. new pr, comment added, build failed), and a culture of checking what else needs review after submitting your own pull request and before starting on the next thing. This also allows static analysis, security scanning, test running etc to happen before the merge, so it's less likely to break trunk. If you cause a problem it only blocks that pull request, it doesn't block anyone else, and you get a notification immediately so you can fix it quickly.
Respectfully, the idea is not to eliminate code reviews. Quite the contrary, working like this should improve code reviews. As he said around 3 min mark this way is the only way where you can review the whole application state multiple times per day - as opposed to pull requests where you often review only one isolated part of the software (excluding other pending pull requests). I think the basic principle is to constantly push code to a production-like environment (not production, especially if you are building rockets etc. 🙂) and use it more broadly from users perspective, while also reviewing the code in pairs while talking etc. As he said -- this is probably one of the only ways to ensure that the changes work with each other multiple times per day. I find this also allows for people to work simultaneously on same / overlapping issues, which is always a hassle with gitflow
@DontDoIt I got that he thinks pair programming is the best way of doing code review, not replacement. Having the person there explaining the changes is extremely helpful when reviewing
@@Blob64bit but if the code reviews are not a gate, it doesn't have to be reviewed at all before it hits production, which is the part I don't like. This just means it doesn't have to be reviewed at all. You have to assume that developers, including junior developers, never make mistakes that aren't caught by their own unit tests, but sometimes there are misunderstandings, or badly written tests etc. You also need to make sure that you do the reviews later and come back to improve the code, but this is just adding a lot of tech debt, and if you can't repay it fast enough, tech debt slows you down. Also, pressure for features often means that going back and improving things after delivering them is seen as a low priority by the business. As far as they care you delivered and now it's time to move on to the next thing.
@@dontdoit6986 he starts by saying he usually recommends pair programming (which I'm fine with) but then goes on to suggest an alternative approach in not blocking the release with code reviews, which I don't like as much, because it means code might not be reviewed at all before it hits production.
Thanks Dave. I am borderline autistic so pair programming is not an option for me. The cognitive load of coping with someone else in my space overwhelms me, so Thierry's solution is an excellent alternative.
I used to do the equivalent of pair programming in electronics engineering where I sat with a PCB CAD engineer and we worked on circuit board layouts together. The layouts came out great, but colleagues that did not do this had poor results with tracks burning up and bad component placement etc.
Feature branches should be rebased each time any branch is merged back to trunk. This ensures that all changes are integrated each time any change is ready to be pulled in. This branching and merging strategy integrates code as fast as it's merged, and as fast as any CI strategy. The thing which many people don't take into account is that there is inherent risk to pushing directly to trunk for the entire team, and time IS lost in editing the DAG to correct pushed mistakes.
This always comes up. If you do this and you have two feature branches created today, and neither of them get merged for a week then they are not integrated with each other for that week. I could delete a class in one branch and you could add properties to it in the other branch. Since neither of them is being merged the issue won't necassarily be detected. It would be worse if it was a more subtle incompatibility.
That is always a difficult problem in sociology. Hard to distinguish, but this does sound a bit like you are dissing the methodology, maybe by guessing what the methodology was? That isn't good science either. Have you read the book that describes the approach?
@@ContinuousDelivery you're right, in that I'm just quoting you from the introduction without going through your sources. Which data are you referring to? The state of devops report? BTW I completely agree with your own methodology and conclusion, combining data and explanatory models. Just stumbled over the correlation part. I'm into the topic because I try to estimate the effect of pair programming in one of my own projects but I think it's confounded by the fact that only easy stuff is done alone - thus outperforming pairs in some way 😅
I'm in a team using Kanban and Pull Requests for reviews. We have status meetings every morning to check the board. 30 minutes. Mostly to make sure that the throughput is constant - following the WIP limits. We constantly struggle with respecting them - since when work becomes a PR in the Review column the developer is eagerly looking for something else to do. I don't like this stress och pushing things through and feeling bad when stuff aren't moving on the board. That ignores the joy of software development.
Sounds like a bit of broken KanBan. Start by removing the "review" column and only have "doing" and "done" columns . Then set a WIP limit in doing. That way no one can start something new unless something moves out of "doing" and the only way to do that is to finish it. Then maybe the CI bit will start to click...
Sounds like the objective on that project is to push items from left to right on a Kanban board, not make better software, faster. Kanban is a very good tool (used it for years) if you understand its objective.
@@ForgottenKnight1 OK! Great! I think some pick Kanban because it is simpler than "Agile" methodologies. Easier to explain what happens, what they supposedly get from it, and get everyone on-board. Leaves little room for complaints. The problem arises when you don't value quality as much as getting stuff done. Or when everything gets reduced to the item on the board, rather than what it is about. I dislike when someone talks about something as "just an item" in the backlog or the board.
Where I work I try to encourage pair programming and any code that is pair programmed automatically passes code review and moves past the review column in the Kanban board. While code that is not pair programmed is gate kept on a feature branch. I'm trying to move towards more and more pair programming. The issue I see with this is code reviewing an entire feature of small commits with other commits in between the ones I'm interested in would be difficult and it'd be hard to track their changes over time to know if they are good or not. Something may look bad only to get fixed in a later commit or look good and get worse in a later commit. Or someone else changed the class in between their commits. I've found that having any kind of code review tends to implicitly encourage larger commits as they are easier to review from looking at the git diffs. On a somewhat unrelated note. We have a very elementary pipeline set up and our unit tests take just a few seconds while our acceptance tests take around 15~20 minutes. Because of this I rarely run the acceptance tests on my machine or while pair programming and am nervous to commit to trunk directly as unfortunately we frequently accidentally break the acceptance tests due to oversights. Any advice on how to handle this? At the moment I commit to a feature branch, wait for it to pass and then merge to trunk later in the day.
I'm not an expert, but from what I understood you would accept the risk of potentially breaking trunk, but need to get build notifications and fix any issues that come up asap with highest priority to ensure it doesn't stay broken. And work to make the test duration shorter I guess.
You could use a method from lean manufacturing. I think it was called the „andon cord“. Translated to the world of software engineering, it would mean: Make the state of the deployment pipeline visible to everyone in the team. (In the old „office times“, we had a screen in the office of our team showing the dashboard of our CD tool which was constantly refreshing - now I have a live version of the dashboard open on an external monitor or getting automated slack messages for status updates). If the deployment pipeline on the main branch turns red, you immediately coordinate how to fix it (mostly the author of the commit will take this or you pair/mob to resolve the issue). In the mean time the team could also work on „prioritizing“ the acceptance tests so that there is a suite of „critical“ tests that can be executed on the local machine in reasonable time and run all the tests including the ones for edge cases (or lower priority things) later in the pipeline. You could also try scoping the tests „by feature“ so that you can run acceptance tests of the things you’ve most likely touched on your machine in less time and run all the tests in the pipeline once the change is integrated.
Limitations of pairing: - time zones (distributed team) - language barrier (team mates don’t share the same primary language) - cultural differences, many cultures have a completely different work ethic and culture.
In that case, you can do pair programming 2,3 hours a day (you're lieing to yourself if you say you'll do it 8 hours a day) Language barrier - this seems to me like a communication issue, so improve that over time. Cultural differences - same as language.
I've worked in aerospace for 8 years now on greenfield and legacy projects and I'll add my two cents. In my opinion, reviewing the requirements and tests before writing and committing code is absolutely critical. As others have mentioned, I don't think tests should be merged to a release branch without a formal review, whether it's a pull/merge request, or everyone sits in a room and goes through things step by step before the changes are pushed to the trunk branch. Adhering to BDD and TDD principles by specifying a feature and the test scenarios that describe it/prove it works as intended, then reviewing with all of the stakeholders should be done before any code is committed because ambiguity will be minimized for all stakeholders. For example, someone without knowledge of the domain or hardware systems the software is controlling may think some utility command is irrelevant and exclude it from the implementation while the operations engineer using the software assumes they'll be able to send that command because the component specification the software will interface with says the command exists. Once that baseline is established and agreed upon, then I agree with everything else you described here provided there is automation in place that couples/traces the execution of the tests back to the same requirements and test scenario artifacts that were previously reviewed and approved by the stakeholders, and that the trunk branch cannot be automatically deployed to a production environment without a final deployment readiness review. Of course, feature/test deltas need to be reviewed just as the baseline. Too often I have experienced the exact issues you describe which lead to unnecessary delays, increased cost, and poor quality software. When you have a seemingly limitless amount of capital and personnel to work with, the old Apollo method of developing software looks and sounds great, especially when the outputs of the process are good enough", but it certainly doesn't optimize for cost and isn't benefiting small space start-ups in these less economically forgiving times. Thanks for producing these videos Dave, extremely insightful stuff.
Honestly, for aerospace, you need to take it slow. The Pair Programming and Agile stuff is suited for non-mission critical software you find in regular corporations. IT/data stuff. Don’t follow this advice for firmware, especially safety-critical firmware.
@@mecanuktutorials6476 Seems more like QA testing issue here more than a code-review issue. No matter how meticulous and thorough the code-review and automated testing process is, only manual QA testing can reliably determine if software is production-ready. Even 100% correctly written code doesn't guarantee software will work as intended. This goes _double_ for mission critical components. And while code-review can be a beneficial preventative measure to avoid potential issues early, when the process becomes too bloated it serves as red-tape that prevents getting timely feedback from proper QA testing. A rapid develop-QA test-rework feedback loop is arguably more valuable than extremely detailed code review. They are not mutually exclusive practices however. Ideally, they should complement each other. Unfortunately, many companies tend to double down on the red-tape aspect when things go wrong, trying to lockdown the process to prevent any and all mistakes within the code, instead of streamlining the process to get feedback quickly as possible.
@@philw3039 I agree. Only manual testing can ever really proxy what the user will experience. In my experience, firmware development is closer to mechanical manufacturing in how it is handled. There is a stark difference between it and Desktop/Web/Mobile which are intended to run on multiple abstracted platforms. A lot of these other fields can “trust” the layer below works and covers ur behind. That trust extends to reviewing other’s work and lets things like Pair Programming happen. You only talk at a high level because the lower level stuff is trusted to work. But in firmware, things have to be more precise. Firmware is the kind of code that really does converge to a requirement. It’s usecase-specific and too costly to shift gears because it is so tied to the system/hardware. Code review is the only way to review details independently and provide precise detailed feedback or concerns. Code review during any development is indeed a hurdle because you’re just trying to make it function. Too much feedback and back-forths become an obstruction to further development. At some point though, the work has to be made presentable for others to read and maintain as well as give others an eye on the state of the codebase. Overall, pair programming or code reviews are not one size fits all tools. Trying to enforce them in all situations will diminish their value. But manual testing is inevitable.
Wow, nice to hear this idea. I just started implementing TBD since December and I found great value to keep a Code Review step but stil do CI. I'm so glad to hear this idea from you ❤
Huh, I had a similar idea a couple of months ago. The idea that we don't need to consider our software "releasable" only after it is reviewed and tested. We could trust our team members that they can write code that works. Using TDD, API-first design and such. It's just that the code might not be of the best quality it has a potential to. That's why reviews could've been done asynchronously or in a pair after the feature is working. Unfortunately, I haven't been able to try this idea in full, since I'm the only backend engineer in my projects. Plus, I didn't have enough courage to suggest making such a "radical" experiment.
I agree with others who might wish to share this sentiment - the channel's weekly videos are lengthy and standalone, making it tough to keep up. Instead of eagerly waiting for Dave's content, I find myself questioning their value (even considering if they're worth my lunchtime). The view count compared to the subscriber base speaks volumes. I propose less complex but still insightful content, with more thematic connections for binge-watching. Avoid churning out filler content to meet a quota. A lesson learned from your own Cyberpunk 2077 video about managing time and scope. One value quality over quantity, a good video is more appreciated than weekly releases only you yourself seem to value. Thanks anyway for the effort, team Dave, loved the book.
I agree with getting code out into prod asap to get feedback, but I’m still still not convinced on the No branch policy. Firstly, just pulling from remote creates a distinct local branch. But more than that, I’m usually working on a few ideas at once, swapping between them over a day. Or checking out other changes from other team members… I don’t know how you could do this without branches…. Maybe with git worktrees… but it’s kinda like working with another branch anyways… further more, doing this my team is still able to deploy multiple times a day… and after, we just rebase and continue. But granted, I don’t know how to get rid of the PR without the equivalent of pair programming before release. But do we need to review in person? Sometimes it’s definitively better, especially for complex changes.
Easy-peasy. The solution to waiting for feature review is not review the feature, but split feature into small tasks which could be reviewed in minutes. I do. No "ready for review" task state, just a message to the "Mearge Request" chat is required. If I'm not on the meeting then response will be in 5 minutes. If I'm on meeting and not currently speaking than the same. Another point that I count releasable any code that compliled and deployed successfuly. Trunk base development with feature switching is expensive, we just open a button or api when the whole feature is ready.
Hi Dave, I do resonate with a lot of what you said for small closed teams and private development, and I do see a lot I personally can improve that can lower frustration and increase value. Now for the elephant in the room, it's not the same for Open Source projects, and currently validation has to be done in the form of pull requests, So things tend to be slow for outsiders of the core team to add code to the project, even high quality code. I don't think there is really a better alternative to that but I would love a video of yours commenting on it. Thanks for the videos.
Fantastic timing on this video! I'm currently acting as an enterprise architect for a very large and well-known company (I'm a consultant for an equally famous global company) and just this morning we were discussing code review and branching strategies for a specific application. I shared a link to one of your older videos on this topic. I can use this new video immediately as a discussion point. I agree with you completely and I'm glad you see other legit approaches in addition to pair programming. I'm working hard to develop an idea that bridges the gap between true pairing and non-blocking code reviews as at very large and older companies facilitating swift change on your personal timeline is a fool's errand.
What data are you talking about when you say process gates correlate with poorer quality? Can you reference the study since it's one of the key axioms of the video?
Claims to have something better than PRs, 2/3 f the way through the video still nothing but talk about how bad PRs are... How about getting to the point so we can shorten the feedback loop on this video?
I recently worked in a project that had a design problem which I was trying to fix. But because of the pull-request & review based release method I had to merge the re-designed code three times, and wait on a review each time. This added four weeks to the release. And made me decide to leave the project...
I'll paste my previous answer... for the 27315th time: "Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches? None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange. What is strange is the obsession with branches to isolate changes and hide systemic issues."
I'll paste my previous answer... for the 27316th time: "Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches? None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange."
Instead of claiming that data shows that pull requests affect quality negatively, how about actually backing up that claim with actual evidence? It is interesting, because Google is also utilizing something like pull requests and they have made internal studies into the effectivness of code reviews and many aspects of code reviews and their internal studies seem to suggest that it absolutely makes sense (as stated in the book 'Software Engineering at Google'). How come that Google came up with a completely different conclusion than whatever data you have?
Well, There are links in the notes on this video to the data that backs these claims and I don't know which data from Google that you are referring to, Google is a big org, but my primary source is the data from the DORA metrics, which is now owned and managed by Google! This is based on the most scientifically valid study of software development practice that I know of, and on the results of 10's of thousands of survey responses.You can read about all this and the reason to trust this data more than anecdote, in the Accelerate book.
@@ContinuousDelivery I have looked through the links in the description, none of the links provides any data about pull request reviews. Which link do you mean? As for DORA data, that's again very vague. Which DORA data point to pair programming better than code reviews? Under what settings? I don't know where to look for the data that you claim you have. I wasn't able to find any data of pair programming vs pull request on the internet, where were you able to find that data?
Dave, could you adress how you would "solve" pair programming in a distributed (time and/or space) team, that for many good reasons agreed on asynchronous work? TY
Hey, Dave! I appreciate your videos and have read your book! I have found both to be very helpful! But I am hoping to get some advice on how do we get the team to that level. How can we get the team to be comfortable with the approach you propose in this video? Currently, my team follows a gitflow and pull request process. Exactly as you mention it in this video. The team is young. Almost everyone has 5-8 years professional experience. Everyone is aware of what a CD process should look like but none want to 'risk' following a new process they do not understand. How can I get them more comfortable with it? Keep in mind that I cannot force them to read your book and watch your videos 😅. As far as I know, convincing your team is a whole different beast.
„Everyone is aware of what a CD process should look like“ Then the work of convincing might be already done if the team agrees on roughly the same CD process and that they also want to implement this (this would be the first question - ask what they imagine to be sure CD is understood by everyone). Changing to CD is not done over night. So maybe discuss some small steps you could take as a team from the current state to move a bit closer to the desired state or process and investigate from there. From there you could improve further or roll back and try a different direction (as it is in agile product development). This might overcome the fear of trying something entirely new. The team might need some help from others (or management) to fulfill some prerequisites. For example the team would need the autonomy to deploy their software independently at some point. - This means the software needs to be testable and deployable independently. - This means the team needs to have the full authority to determine if the software is releasable or not. … and so on. You might need to convince a tech leader in the company to follow these kinds of goals (hopefully you don’t have to convince for these goals but my experience tells a different story 😅). So do changes in small steps and you’ll get there at some time. If you need help from a person with more experience in this way of working, asking questions (or for help) about these tiny improvements will also give a more detailed answer.
Hey Dave, right at the beginning you refer to some kind of data that states the correlation between process gaps and quality. I'd love to see that data. Could you give a hint where you found that? Thanks for your awesome content!
Also trunk development would make the team members start to own the whole codebase and not only their own branch. But yeah, that requires maturity from the members. But maturity comes with responsibility. So, kind of a chicken-egg problem :) Regarding the situation where you need to roll back changes for the feature, and imagine, you have several team members contribute their parts to the feature, like backend, frontend and e2e tests, you may get into troubles, having to search for every single commit related to the feature, and removing it. And if those commits contained any unrelated changes, this may break the product. To overcome this, you then also need to have a rule like, that feature releated commits are clearly separated from any other code improvements you want to make to the code. Well, again, that requires high maturity and sense of responsiblity from the team members. But yeah, i can confirm, the merging hell with feature branches is real. Another scenario, how do you make breaking experimental changes with trunk based development? I see no other solution than experimental branch, but that would be much less frequent event, than merging hell on a regular basis.
yeah, you're actually bringing up a real life case vs. the ideal ones that Dave used to showcase his ideas. I mean, on a living system, when you need to change a feature (and if it is also a dependency for others) what do you do? I think i will anserw myself, that one needs to actually create a new feature, not change the old one, then use feature flags to switch one for the other when ready. still, this is most often hard to sell as everyone sees such aporoach as more effort 🤷
It will be awesome to know your opinion about what I think is the biggest issue happening to programming nowadays. What do you think about the common practice of letting Business Analysts and Stakeholders design software? In every project I worked (except for the small ones) Business Analysts and Stakeholders design the application till the most tiny detail and that has a lot of impact on how you implement that solution. A small detail in the UI can impact even your DB Model, in the end people without any technical knowledge is designing the architecture of your system. Is very common to see UX designers closer to the Business team rather than the development team, and that is awful. Also this practice is taking all the fun out of being a software engineer.
@@ApprendreSansNecessite In my case, UX/UI designers present the mockups to the business before presenting them to the dev team, and there is a lot of bureaucracy required to ask for a change
@@leonardomangano6861 how can the business force you to couple the database and the UI? What lead to this? I am not questioning your testimonial, it is just so odd to me.
@@ApprendreSansNecessite It's not that the business forced me to couple them. The UI and the database model are coupled by definition. The design choices you make in the UI will impact your DB model
@@leonardomangano6861 UIs and databases change at different rates and for different reasons. You cannot blame the business or the UX designer for having failed to put an architectural boundary there.
With "Ship", there are no code reviews at all, is that right? Cause then, in this aspect that could be even more "radical" than what is described in the video (where some kind of code review always happens at some point if I understood correctly), depending on what kind of changes the team decides can be "Shipped" and what should be "Asked"/"Shown".
Pair programming is expensive. Up to x2 the time (and developer salaries) will be spent on developing the same functionality Similar thing can be said about TDD. It doesn’t always make sense spending a lot of time on covering all of the functionality with tests. Sometimes the cost of error is low. And it would be much cheaper and effective to get the feedback from user experiencing an error or see it in production or staging sentry (like log but error aggregator)
I hate when people push pair programming. My small team is spread between west coast US, east coast US, and Eastern Europe. How the heck am I supposed to shoehorn that in?
Question: you say there is a negative correlation between proces gates (like pull requests) and quality. Has this correlation been controlled for other factors like project size?
Overall there is a deeper survivorship-bias in the underlying statistical analysis of fewer "blocking" quality gates (like PR) leading to higher quality / delivery performance. Accelerate does go a little into this when analyzing the DORA data, but the focus is on other parts. Still, it should be understood that highly efficient / capable teams should not get blocked by such gates. But there are also less capable teams, or inexperienced ones. Or ones that only start to form skills and trust. In those case removing most gates is dangerous. But we do not get to see any data from such teams, because the company will either hire consultants to fix the damage or go out of business. Therefore we do not see any data from those failing teams in the data. To deal with this bias it is better to start with great teams and get rid of the training wheels one at a time. Given enough time, skills and trust within the team will increase enough for them to gauge quality their own way. That is when to switch to Trunk-based development, not before. The extreme worst case in comparison: a majority of the team is both new and unlikely to stay for more than a few months. Do you really want to switch to asynchronous quality checking of their code (which the video is basically about) if they're unlikely to care for any bad consequences or damage to their reputation? And if you actually weigh by teams instead of projects, this kinds of fundamentally broken teams are just way more likely to happen because highly efficient teams are so efficient they can do a lot more projects within the same time. So if projects follow a normal distribution overall in the industry, there must be more bad teams than good ones because bad teams not only have worse projects, but also deliver fewer successfully. So please do be wary of this bias overall.
14:12 Before merging feature to develop, develop branch with verified changes of other developers should be merged to the feature branch. After this CI verify all verified changes of other developers plus not yet verified changes of the feature branch. There are the settings in git providers that doesn’t allow to merge from outdated feature branch.
How would you advise to work in an environment such as a game development team where some people are working on a game and others are working on engine features or lower level features of gameplay? having worked with a CI oriented team in such environment, it could be challenging to have parts of the engine broken for several days when you were developing a gameplay feature... How to accommodate game designers that need a kind of "LTS" version of the tools they use with the need of continuously testing and breaking things of the engineers building the tools? Would you say CI is inadapted to game development?
Well, sorry, but it isn't CI if there are "parts of the engine broken for days"! The aim is to make small changes that keep the SW working after every small change, commit them to check that these small changes work with everyone else's small changes and if one of your changes breaks something at that point, you fix it to revert the change immediately. CI is about committing WORKING SW multiple times per day, it is a more incremental way of working. CI and CD both work very well for game development. It takes a bit of adaption in the way that you work, and it sounds like the team you worked with hadn't done that.
Of course your Kanban board should have queue columns - that's a given for the pull system and absolutely not mutually exclusive of PRs. This video is just the same 'ol recycled contortions propping-up anti-PR dogma. PRs can or can not be trunk-based development. PRs can or can not be CI. Its like any other tool and depends on the use/implementation. Saying PRs can not be trunk-base or can not be CI because a few cherry-picked anecdotes bites the Dogma disclaimer at the beginning that was intended to preemptively claim this anti-PR dogma is not dogma. Feels a lot like FUD to sale a training course and/or youtube ads (and yes, i got suckered thinking there would actually be some new info). If not FUD or bait, lets disambiguate. How much time can code be non-integrated for it to still count at CI? is one day? how about 1/2? Share the bullet points of the alternative that "hides" code materially less than what is possible with a PR. The process I like, that uses PRs , is auto merged to main on green and since it is fully tested and deployed outside of main, including with main's code, there is never the waste of a roll back. Only when someone has doing some thing "creative" with git is there ever a merge conflict and since the conflict is in the PR's merge branch, it keeps main in a deploy-able state.
Thank you for inspiring us to taking our processes to the next level! As you suggest, we also switched to trunk based development completely and we are happy with it so far. As a consequence, we do have a question about deploy version management: If our CI/CD server now creates a version bump automatically, we would need to also commit this change to VCS, essentially creating another automatic commit for each pushed commit. While this does work, it does not seem to me to be the best practice. I'd be curious to hear your thoughts / opinion. 😄
I am pleased, but not surprised, that it is working for you 😊 There is no perfect answer to the version number problem, other than discarding 'semantic versioning' which isn't a great answer either, because it is useful to communicate with customers and users. My recommendation is use a generated unique id as the 'real version number' or 'release-candidate id' and treat the semantic version as a kind of 'display name'. You can keep a record associated the display name to the correct version number somewhere else and so not compromise the VCS of the release candidate. The problem with this is when you need to package the display name as part of a binary delivery. It is largely not a hugely risky thing to add it as a final packaging step, but I prefer the purity of 'test what you deploy into prod' if you can manage it. So a compromise, but not a horribly risky one.
@@ContinuousDelivery Amazing! Thank you Dave for the detailed response. 🙏🏽 (I'm in the same team as @coZhenya.) I love the idea of treating the "user"-needs (version number display) and "tech"-needs (unique-id) separately! I admire the consistency of your applied mindset and context-aware openness that I see reflected in your reply. => Why only use the single responsibility principle in code. 👏🏽 And yes, you anticipated correctly that I want to inject the version(s) into the build so users can supply that information for debugging purposes. I concur with your preference of bumping/generating the version(s) prior to testing to keep it pure/clean. Since I'm rather new to trunk only development: Would you be so kind to elaborate on your thoughts about the double-commit challenge? Is it perfectly normal to let the ci/cd pipeline commit the changes so that a future release is aware of the last version? Or was the idea that using a generated id replaces the need to "remember" the previous (non-user-dispaly) version? Thanks for sharing your intelligence about the ability to change. ☺💙
@@coDocumentary My preference is to use the "real release candidate id" and only associate the "semantic-version-display-name" once the pipeline has approved the change. So I have done this, in the past, with a separate step and separate, tiny, stage that 'attaches' the version to the release-candidate. In one version we patched a file, designed for the purpose, with the display name version number. It depends too much on you tech to go much beyond that as advice. I think what you are looking for is the minimally intrusive way to 'attach' the display name to the code.
@@ContinuousDelivery Perfect. I appreciate your advice and it is enough for me to get the idea. I was a bit hesitant with patching the code at that point, however your strategy and the reasoning behind it does make sense to me and I feel more confident to commit to testing/adopting that approach now. 🥰
Pull requests help track the pieces of code to review and make it possible to comment and all that jazz. Do you have suggestions for an alternate flow? The review column is what I would reach for instinctively and I guess semantic commits help a lot but doesn't it feel a bit messy / inconvenient?
Tools like Jira/Bitbucket will cross-reference commits to tasks. If it is the task that is progressing into a needing-review state then you can find the relevant commits directly from the task.
strongest issue with this assesment is that developers are more often than not: inexperienced, unskilled, messy, chaotic, conceited and arrogant. It's not that it's intentional but developers' ego are something that is making things like CI/CD is effectively impossible.
Dave, I love community comments and polemics under your content. Maybe it is a matter of not being a native speaker or but I understand and learn more from comments than from the actual presentation (visual side is great, but maybe sophisticated speach strictire or presented concepts are too general aka high level for me, a simple dev?)
Another problem I can see is people. Some may just feel uncomfortable knowing their code can go to production without a code review. The process allows to account for that by doing on demand pair programming of course, but this just goes to show that it is not suitable for everyone. The team has to be fairly mature I think.
I always wonder, and can't quite figure it out, is it possible to have different people write tests for your code and still do CI ? If someone else is going to write, let's say, integration tests, then there's always some code in trunk that has not been tested yet (but all tests pass because tests for new code don't exist yet). So then you can never trust your CI, because you never know if tests for all new code have been added yet. And even if you knew that, testing is probably always a bit behind development, so you can never release.
It's always so cool to hear all these great approaches to getting quicker feedback and learn from each-other in the workplace. But I sometimes wonder how this will work in a 2 people team or a team of just-starting-juniors and only a few seniors or only one or none. Won't it be nearly impossible in these situations?
Pair programming by rotation. Depending on the size of your team, you might have to do multiple permutations before all seniors have worked with all juniors. There are plenty of videos on YT on how to do this.
Pair programming and TDD are being rejected for some reason. I wonder why? Maybe because I don't have a bunch of Kent Becks working for me. If I had, I wouldn't need pull requests and code reviews either.
Maybe, but another way of looking at it, is how do we create the next generation of Kent Becks to work on our team, I'd say through Pair Programming and TDD.
@@ContinuousDelivery I can't agree more. Extreme Programming Explained is a great book. However, the programmers I work with don't have skills to pull off TDD. And if you put two juniors to pair program, you may end up getting the double spaghetti code 🙂 Just yesterday was reviewing code. Test passed, however the way the code structured there was a potential for concurrency issue. Had we released it in production we would have errors happening once in a blue moon and we wouldn't know why. Reality of business and scarce resources forces you to work with what you have.
How many times have I carried out a pull request code review and the code is not how I would want it but it works and re-writing would be too costly. Pair programming would have caught this at the earliest stage and there's your quality built in.
$0.02/wo: The PR seems to have come from organizations that have a particular management style and culture (Google?, and perhaps Linus) where the idea is that "you are not worthy" to commit code to "my repo". So you are expected to doff your cap, bow, and present your miserable code for blessing by the priests. I'm sure this works fine in that culture. But most of us don't have that culture. We hire developers that we expect to be competent. We fire them if they're not competent. So the whole notion of bowing and scraping to be allowed to commit code is not applicable. Instead we might want to have a manager or dev lead _look_ at code being committed in order to catch bad code and maintain their list of people to fire or send for additional training, but there's no need for a review-delay-gate. We might also want to have a formal code review process in some circumstances, or for some kinds of code (e.g. security-related). I think this is roughly what Dave is describing in this video. The PR isn't designed to facilitate that, but there is a need for some tooling support -- a kind of "code queued for review" feature in the sccs -- otherwise how would we know what code had changed in the development of a particular feature, after the fact?
I agree with the core of your point, perhaps not the cause, I think PRs come from the open source world, where it is sensible to be a bit wary of unknown people committing to the core of a widely-used operating system (for example).
We do this but with a set of different rules. All code goes on to trunk reviewed. Changes should be small, not nessescarily a whole feature. The code that goes in is releasable meaining the release artifact works. Only thing that blocks the release should be code design changes and code that doesn't work. Everything else can be fixed later. This approach works quite well. I know pair programming is better but the human side of pair programming makes it too unpredictable for me. Probably if you do it disciplined enough it would work but I just can't concentrate if someone is looking over my shoulder. I guess if you have a good process like ping-pong it will help it but it takes too much training for the other guy to stick to it.
PR's has no direct relation to quality, but to two other reasons. If you are using PR/MR for quality, then you are doing it wrong. The reasons are, first, the code been ready this happens because you need the team to be able to develop things without changing the behaviour of the mother branch, until you have finished your work, allowing that the system could be validated in its isolation(No other features intervention). The second one is related to the fact that not every developer is Senior. Companies have a lot of people on different maturity levels working on the code, so you need to review to make sure that code directives and patterns are been followed.
They have a measurably negative effect on quality if they slow the feedback process (CI). You other recommended uses for PRs are also better fulfilled by other ways of organising code review, pair programming and continuous-review (described in this video). Finally on your final point PRs as a teaching mechanism do have a place, but are very poor second compared to pair programming.
My Question about this is what do you do with unfinished features tho? Releasing unused code into production is a pattern I thought was considered bad?
Depends who you ask I suppose, and what you mean by "unfinished". There are ways of working that make this safer than the alternatives, this is how SpaceX work for example. The key is that unfinished does NOT mean un-tested. All the code released will be tested and working as designed, though it may not yet, add up in some parts to a finished feature.
A couple of things stand out: It seems like you're referring to human-gated pull requests. I'm not sure that pull requests need to involve a human. You could have a series of fast automated checks and, upon passing those checks, merge the code. Some examples of checks would be verifying no linter configuration changes, running your linter/formatter, running static analysis and having no new (major) potential vulnerabilities, running automated tests and passing, no newly disabled automated tests, changes introduced have line coverage. This could also give you a good list of things to review. If your tooling allows you to find merged pull requests that have no human approvals, you can review the change. There's always a chance that future changes could change the context of the pull request, though. Minimally, it points you to the work done that hasn't been looked at by a human.
I build all of the things that you mention into the commit stage of my deployment pipelines, this is CI not really a PR, and I'd argue that PRs don't really add anything at that point. Why bother creating a branch, when you already have one in the form of 'my-local/main' and you can trigger all the evaluations that you mention on push to 'origin/main'?
@@ContinuousDelivery I suppose if you have tools that allow you to use hooks, you can run the checks and rollback the merge. Not all repository hosting services allow for server-side hooks, though. It just feels cleaner and safer to have a remote server with controlled configurations run these checks to avoid tampering. Someone independent from the development team can control the configuration of the tooling used to execute and approve the automated PR checks to ensure that the checks are in place and can't be overridden by the developer of a change. It lets code get into the mainline faster. I'd rather make sure that all the fast required checks pass before integrating and defer some of the more intensive checks to post-integration (like a more robust suite of automated tests, dynamic application scanning, and so on). That isn't to say that I wouldn't rather do pair/ensemble programming and have a robust CD pipeline. This could be more in risk mitigations for cases where you need more human monitoring or involvement in the process.
Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches? None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange. What is strange is the obsession with branches to isolate changes and hide systemic issues.
@ddanielsandberg Feature branches should be merged in at minimum once per day. Thus, everyone's changes are merged and conflicts are noticed. It's pretty straightforward.
So this model doesnt allow for bugs because we have tests, but if I write the code and the tests, then any bugs im not aware of will be in both. We make sure reviews are done by separate people but the tests (that we rely on) aren't held to the same standard?
No, there will still be bugs, there is data to say that there will be about 60% few bugs in production if you adopt a good testing approach, there is no data that I am aware of that indicates that PRs reduce bug count, and in fact when compared to more flow based practices, like CI, then the data is very clear, and quoted in the video, based on the DORA metrics of Stability & Throughput, teams that practice CI score a lot better on both stability (a measure of quality) and throughput (a measure of efficiency). If you practice TDD, which is my preference, you are less likely to have bugs in tests, because the process includes a validation of the test - we always run the test first, before we have created the code to make the test pass, so that we can see that the test is actually failing as we expect.
I will never allow unreviewed code into the open. The moment you are dealing with anything important it has to be seen. If you write small commits for big things sometimes you lose context. I see why ci cd dislike feature branches but this system seem like it introduces entropy and buried errors.
Thanks! But you should work on the slides, its not needed to see you on full screen all the time, its more important to see diagrams and text (for longer periods of time) that explains the main points you are saying. As it is now its hard to understand your point.
Hi Dave, I agree to your arguments but there are cons you do not talk about. Trunk based development fails if your collegues are Not trust-worthy. It gets a big mess if the work is not done as expected. Lets say the developer does not write tests and adds buggy code. It gets even more evil, if they act like to sabotage the work. In those cases you need feature branches to limit the damage to that Branch. Somewhere in future when the team is formed you can move back to trunk based. It would be nice, If you can talk about this more often because in reality, it takes so much energy to get your team to that sweet spot of CI you are talking about.
@@Blob64bit My personal opinion: Running is no solution for me. I use just use different tactics on different situations. The goal is get the team in shape. I had success so far. If the team is in one level, you can get back to what Dave said. And you can be proud on your team. Anyways, you have to deal with all kind of people, even those that add negative value.
I think too many assumptions made here. We use pull requests to scope the change(s), which are automatically linked to the issue for documention for the future. Nobody said pull request approval has to be performed by a human. We have more than one approver for a pull request and non of them are human and they are all quality related. Sometimes you can sync to a trunk way too soon which massively adds to work. ...and so on and so on... The assumptions about how people use pull requests are too many.
This video asusmes that the developers in your tram are good. Around 80% of devs i worked with for 20 years have been mediocre at best. Try doing a medical device without code reviews / PRs. Of course this approach can be improved, but after reading the full trunk baded development psge it was clear to me thst it is not for sny of my teams. Love tour videos by the way!
Get introduced to the concepts, benefits and key practices of Continuous Delivery, and develop your knowledge of the first steps to start building better software faster. This course is IDEAL for people who are new to Continuous Delivery and non-technical people who want to understand more about CD. Study the fundamentals of Continuous Delivery FOR FREE HERE ➡ courses.cd.training/courses/cd-fundamentals
What is spaghepticism?! 21:00
@@Sergio_Loureiro when you're not sure if it's spaghetti code or not :D
Dave starts explaining the technique at 17:25. My summary:
- team must use trunk based development with continuous integration
- code is written with small commits and merged to trunk when complete, then the ticket is moved to a 'ready for review' column on the team's kanban board
- code is reviewed when someone is between pieces of work at the start of the day
- unreviewed code may be released, however the CI process provides confidence that the system will still work
Didn't get to that point, but no mention about Feature Flags?
Thank you
I think this process could work for some but not all. Depending on your product, ci cannot catch everything. There have been many times where my team mates have helped me catch something before I merged /released.
Having a phased rollout I think is really important. Use your user base as part of your regression tests instead of trying to ship perfect code.
Step four: unreviewed code is released. Step five: no-one reviews it the next day because they're being stamped on by the business to "deliver" features.
I have a strong believe that if people want to work with trunk based development, they should stop using GIT and go back using SVN.
Our code reviews (as pull requests) also find gaps in test coverage due to developer inexperience. So, we can't safely argue that the tests contained in unreviewed code provide us the surety of finding/preventing bugs, because there won't have been any oversight to ensure gaps don't exist in the added tests. I have worked with engineers who are so experienced that you could safely drop code reviews, however that is not the majority of engineers; if you have any team members who aren't in the top tier of experience and ability, then allowing them to commit to master and calling that releasable as far as I can tell, sets you on a path where it is a matter of time until you break production because they release a bug alongside test code which insufficiently covered the bugged code in question.
Totally agree. Also If you are a single developer, those merge request are gold to review yourself.
most companies also run automated quality checks that run as part of PR process automatically. If you pair, you pair is also human it does not catch everything, you can easily forget to run the necessary checks before committing, and git hooks don't always work well with any build tool (works ok with husky / does not work well with gradle for example)
Test coverage could be verified by an automated pipeline which should mark the code as unreleaseable if the coverage is not enough. It would not check the quality of the tests, though, unless you'd use mutation tests but those tend to be slow.
I still feel safer if the verification pipelines fail before my code is the main branch, therefore short-lived feature branches and PR's still seem a good option to guarantee that:
1) main branch contains only releaseable code and can be deployed immediately
2) I don't always have to run the whole test suite locally before pushing my changes. If I missed somethiong, I get the feedback from automated pipelines before my changes affect other teammates.
I'm aware that it might be the old habits speaking, though :)
I have seen places where they try this. The problem with those “small changes” continually merged into prod is that over time code becomes a pile of patches one on top of another , each managing to hit its target but not taking care of the overall code layout.
The better the team, the longer it takes, but eventually everyone justifies these hacky approaches saying that they cannot refactor the whole thing and the thing needs to be rewritten from scratch. But even in top teams, after two years of practicing this, the mess is unsalvageable.
Simple problems need simple fixes: if you are afraid of big merges , simply merge your branch with main everyday.
@@Sjwnbszhbsjnbvvs +1 to that! I frequently review my own PRs (whether I'm the sole developer or not) to make sure everything is as I intended to check in. I've caught many potential bugs and accidental file check-ins like this.
Who's writing the automated tests though? If nobody is reviewing the tests you don't know if the tests are satisfactory. If the tests are wrong or missing cases you're immediately in trouble without reviews.
You mentioned the data says pull requests are worse than trunk based, but who are the subjects of this data? High level teams with top talent might be able to release without review, but in many companies it won't work unless someone expirenced reviews the code.
Pair programming is at least doubling the cost of a dev team. As you already said, companies aren't going to do it. The data isn't strong enough.
I was just on a project at a US Fortune 500 company that is actively doing pair programming. I had never seen it in action before. Now I'm in favor of it. The review process happens concurrently with the development.
Yes, it can interrupt 'flow' and it takes some learning. But I was impressed with the results.
I also want to know what this data is. Can you provide your source @ContinuousDelivery?
"Who's writing the automated tests though?"
- There are a number of ways of addressing your concerns, for example, you can have automation setup to flag code without tests and possibly use something like AI to help more junior developers write code. The goal here is to keep code changes small so that it's much harder to miss possible cases.
"High level teams with top talent might be able to release without review, but in many companies it won't work unless someone expirenced reviews the code."
- Why would this not work in many companies? Do you mean that many companies don't want to change, if that is the case then it's a totally different question. The question I'd ask is have you tried proposing this change? In practice changes like this are difficult to introduce and require a lot more (and I mean a lot more) than one meeting with the stakeholders and may take months or years.
"Pair programming is at least doubling the cost of a dev team."
- There are various papers that state that you do lose output when pairing, about 10% as far as I remember, but you do gain a substantial amount of quality (and other benefits) as a result. This goes back to introducing changes in the company, which is not easy.
Pair programming is a makeshift and outdated reaction to rapid growth and change in technology. It's an adhoc tactic, avoiding investment in mentoring, training, and distributed engineering practices.
My two cents worth :)
"Who's writing the automated tests though?" - the engineers that develop the feature.
I think the code review process solves the wrong problem. Usually we have this gate, because we don't trust people, or we are not sure if they produce good quality code.
The better solution is to teach people how to write good code. My team does this through pair or ensemble programming and regular coding dojos for the whole team, where we learn how to break a problem into very small chunks (usually with 5-7 minutes of work). We learn from each other, we learn how to communicate with each other, we share ideas, we discuss the code, the consequences of doing something, the architecture, etc. I think the discussion is one of the best learning tools.
The next problem with code review is that you can always say, it's not my fault, those guys checked it and didn't see a problem. There is a lack of responsibility, and I think a lack of trust that less experienced developers will merge something stupid into main.
Another thing I don't like is that code review is sometimes harmful. Imagine you have been writing code all day and the other people say you have to rewrite it completely. It's demotivating and cruel. Instead, we should write the good code from the beginning.
My team does not always do things in pairs. We've allowed ourselves to push and release code without someone else checking it, and we haven't had a bug in over a year. I trust my colleagues because I know how they work. They use TDD, they write tests first, then code. I know that if anyone has any doubts, they will pair up with someone else.
Long story short, you should build in quality into the process and build trust.
This non-blocking review strategy definitely seems interesting to me. I feel tempted to introduce it and use it as a back-door to getting us to actual pair programming. :)
The data about post-hoc inspection has been around for decades:
1) Cost of Delay
2) Additional labour cost
3) Defect rates increase with amount of inspection.
So Pull Requests "solve" (ehum) one problem, and thereby institute three new problems.
Back in my Six Sigma days, we always used to say that "an ounce of prevention is worth a ton of cure," i.e. if by any means you can improve in-process quality, you can reduce the cost of poor quality by orders of magnitudes.
PR's only have a place where you don't have control over in-process quality, i.e. open source projects with unknown contributors. In all other situations, in my opinion, PR's are a sign of QA not doing their job (i.e. figuring out why things go wrong in the first place, and initiating improvement there.)
This is an awesome answer and mirrors what I've been trying to say for years but lacking the language to do so. (commenting to have a bookmark)
For me the PR process is not merely to prevent bugs or ensure tests are passing but it's much more than that. I see a quality assurance aspect here that is far more important in terms of ensuring the code follows a common shared theme/style and approach (tools are available but it's more of a shared software team vision). As a Tech Lead I don't want to have to spend future days unravelling and refactoring someone's code that was poorly implemented from an architectural point of view (even if it is bug free). I'd rather shift left and identify any architecture creep early in the PR
Yes! Yes! I worked for a feature phone platform company that put all of its competitors out of business (before being acquired) by using a similar approach. We cut the development time for a feature phone from a year to 3 months. For the most part, we communicated through code - both locally and remotely.
Great video, thank you Dave. Identifying that anything that introduces a gate in the software delivery process carries a cost of blocking a continuous flow and we should think if there are better ways to achieve the outcome. Code reviews definitely fall into that category. Some comments correctly point out that code reviews do catch issues like security holes, incomplete test coverage, missed use cases that will affect quality. I would argue that if you are currently relying on code reviews to catch these, it is already late. They should be caught before the code is written.
Just a thought about merging before code review, while I fully agree that the purpose of the review is to ensure code quality and not to catch bugs there could be other potential issues. In my experience sometimes more junior developers may introduce potential security risks with their code and if those make it to production this can be catastrophic. Security risks often can not be caught by tests or QA, only a pen test will discover it or if you are unlucky an actual attack.
I guess junior developers can be assigned as not trusted and as such will not be allowed to merge without a review or have to always do pair programming but this comes with its own challenges.
Such a mistake is not also reserved for juniors, anyone can introduce security risks without noticing.
I am not trying to bash the idea, just some perspective
Labeling juniors from the start as 'not trustworthy' will significantly damage confidence and motivation. They will have a hard time trusting their own abilities and won't share their thoughts on the project ultimately preventing them from becoming a good developer.
This sort of elitist thinking is what causes a lot of problems in software development which I think Dave is trying to prevent with his approaches.
@@mrspecs4430 Yes I agree, this is why I wrote "this comes with its own challenges". segregating the developers like this is very bad for the team as a whole and could hinder their own development.
More experienced devs are also not immune from making mistakes as well.
My point is that nothing comes without some side effects, second pair of eyes in a code review is quite good for everything else except that it's inherently a bottleneck. When do away with it we increase the the speed of development but we are exposed to more risk, saying that we are not is simply not true.
In a sense, like with everything else, there is no one superior model that is the "best" for all teams and companies. I think people should look at the options at hand and the implications and choose what fits them best as opposed to follow blindly some model of working
@@mrspecs4430 On a side note, labeling junior devs as 'not trustworthy' will probably have the effect you are describing for some of them and the exact opposite for others depending on their personality because some people might push harder to achieve the worth status.
We can't know for sure just by guessing, as this will be pure arrogance.What I do know is that it will be an environment that I wouldn't want to work in.
@@mrspecs4430 whether you specifically label them as not trustworthy is not as relevant as how you end up treating them. And you will end up treating code from junior developers as less trustworthy, because quite frankly it simply is. It's only correct to make that assumption. That's why they're junior developers. If their code would be trustworthy, they wouldn't be junior devs, they'd be senior devs. The lower trustworthiness of their code is implicit to what it means to be a junior developer.
@@aaronhamburg4428 I agree there is no silver bullet approach but it's important to re-evaluate our common conventions and recognize our own biases from time-to-time. For example, if security is that important to the project wouldn't a more robust solution be to:
1) Document common coding security anti-patterns and risks and make sure new developers are aware of them as part of the onboarding process
2) Require that all developers earn and maintain some kind of certification such as Security+
3) Use code vulnerability scanning tools to ensure major security risks are identified.
You could still choose to have PR's if that works for your team. Code reviews have several benefits. However, the notion that security risks are a "rookie mistake" and the only line of defense is a wise, hawk-eyed senior dev making sure the code stays pristine seems a bit biased to me. That strikes me more as something to ensure a pecking order than to truly benefit code security.
I agree with a lot of the commenters who say that one thing code reviews catches is poor tests or no tests. But also this approach assumes you already have test coverage, which may not be the case, and I think this would be disastrous in that case. In addition, there's the cultural problem that product always wants more and more features and so if you have code that "works," you might have to fight pretty hard to get the opportunity to go back and make it better--even if you caught that it needed improvements right away. Having that approval gate lets developers conceal that process from the business so code quality can be maintained.
So true about code reviews being about code quality and not catching bugs. At my company, we used to do code reviews after code was checked into the mainline trunk. We have since then "upgraded" to using git PRs and now the changes are tested in our CI/CD pipeline much later than before.
It’s mostly to ensure the checked in code is maintainable by others going forward.
Cleanup after checkin is unrealistic because there won’t be any time for that later.
@@mecanuktutorials6476o true, when it comes to code clean up, later often means never
@@mecanuktutorials6476 "Cleanup after checkin is unrealistic because there won’t be any time for that later." - says who ?
@@ForgottenKnight1 once it’s checked off as “done”. There’s always other stuff that is prioritized ahead of refactoring the older submissions.
@@mecanuktutorials6476 Then fix that, because THAT is the problem. If you realize you're not taking time to improve code quality, then how does imposing PRs and code reviews fix that? You still have developers that cannot be bothered to write code well in the first place.
I'm so sick of these excuses. All these processes make for WORSE developers. "Oh I don't have to write this well because X will tell me what I need to change anyway" How about becoming a better dev and writing things readable in the first place? How about taking responsibility for the code and if you see something that's not well maintainable you fix in righ then and there instead of writing someone else a "review" so they have to fix it.
All this crap is just lazy bandaids and damages both developer growth and a feeling of teamwork and trust.
I personally hate pair programing, It prevents me from ever getting into "the zone" ( Pair debugging legacy code is another story)
Same. Very hard to get into those "10x moments" during pair programming.
Pair programming imho is just a fancy name for "training a colleague".
@@FudgeYeahLinusLAN these 10x moments usually end in bad code because you end up coding for hours on end. Pair Programming gives you a framework to work on code sustainably and with a clear mind. You switch every 10-15 minutes and take a 5-15min break after you and your pairing partner had their turn. This is in my experience produces way way better code, removes the need for review & is much more sustainable aswell as a more sociable experience which is a plus if you have nice coworkers.
I prefer pair reviewing, because all the back and forth with comments gets old really fast and is a place for festering passive-aggressivenes 😁
There are two states in which we *should* be operating:
1. Complete trust, so there's no need for pull requests.
2. Building trust, so we should be working very closely together to ensure we're learning each other's strength and weaknesses: i.e. pair programming.
If you think about it, pull requests don't make a lot is sense, except for open source.
While at Thoughtworks, we used to have Code reading sessions in my projects. So instead of reviews at PRs, we would go through the diff weekly once, entire team. And let the team think about what can be improved. And if something was worth improving, we would do it during next change. And everyone now knows and agrees to the need to change.
That'd take days!
I started my career in programming long (21+ yrs) ago doing programming in pair with a product manager. It was an incredibly inspiring experience that sparked my interest in IT career. This is why I strongly agree with you, Dave, and believe pair programming not only delivers great value but also provides a cool engineering experience.
second it! Sadly I found myself in such situation only in 2 companies I work for... current gig is a disaster from dev processes.
I don't see how working on the same branch magically avoids merge conflicts. Eventually all the changes need to be merged. Instead, merge conflicts are reduced by keeping each change small, irrespective of whether you use PRs or trunk based development. Since I can't use pair programming (open source, different time zones), what I do is if the PR gets too big I tell the author to split it into multiple PRs, and try to have enough work queued up so they can continue doing something else while waiting for a review. Also I tell people to regularly rebase, not merge, the main branch.
His only answer is "buy my book".
If you split something into multiple PR's that depend on each other and then someone suggests a change in the first one you now have to rebase all the other ones which is annoying and wastes time. Rinse and repeat for each other PR until the last one. And the longer it takes you to merge the first PR the more out of date with trunk all the other ones get.
What he advocates for is yes, split the work into small parts, but commit each part to trunk instead of waiting for reviews in a PR (he advocates for pair programming so that each commit is already reviewed anyway).
@@ricardoamendoeira5689 I understand, I tried pair programming and I actually like it and see how it can improve the process, but I can't use it since there is not enough overlap of time availability. I also understand it's better to preempt a PR from growing big instead of telling to split it later. But again I don't always have these options.
Dave, thank you for your great advice! I just want to point out to others that feature-branch based code reviews can still be an improvement to lesser strategies. For us, it was. We have a bunch of developers but also a large code base, so we don't run into merge conflicts very often. Most of the time they are easy to fix. And when we merge, that very rarely breaks anything. While it's not perfect I think our amount and quality of testing is reasonable. One little thing that helps a bit is that the CI/CD platform we use always runs the build and tests on a temporary merge of the feature branch with the trunk. I know, that is also not optimal, but better than not doing it :) Yes, we can improve and we should sooner or later. But we came from a worse place.
I can go with pull requests or pair programming, but I'm not convinced by review after release, where nobody has reviewed the code before it goes out.
I think this is more likely to lead to bugs, and as we all know, bugs slow you down in the long run. Higher quality up front is faster.
Code review is also a great time to break down knowledge silos, and for junior developers to learn.
Also, the tests might pass but they might not be right. Even with TDD, people make mistakes. The tests might test one thing but actually it's not the correct behaviour because of a misunderstanding, or a junior developer might be new to TDD and add more than the minimum code required to make the tests pass etc.
There's also performance. Performance tests are great to have but they can be slow to run, giving slower feedback, and it's quite common to catch performance problems in a code review before the performance tests.
Where I work we do pull requests with a minimum of 2 reviewers, and the average completion time is about an hour. In that time, you can keep working on the next thing, so you're not really waiting at all, and you don't deviate too far from trunk. We can still deploy multiple times a day, but what we deploy is higher quality as a result. We do this by using a pull request dashboard for visibility, with desktop notifications (e.g. new pr, comment added, build failed), and a culture of checking what else needs review after submitting your own pull request and before starting on the next thing. This also allows static analysis, security scanning, test running etc to happen before the merge, so it's less likely to break trunk. If you cause a problem it only blocks that pull request, it doesn't block anyone else, and you get a notification immediately so you can fix it quickly.
Respectfully, the idea is not to eliminate code reviews. Quite the contrary, working like this should improve code reviews. As he said around 3 min mark this way is the only way where you can review the whole application state multiple times per day - as opposed to pull requests where you often review only one isolated part of the software (excluding other pending pull requests).
I think the basic principle is to constantly push code to a production-like environment (not production, especially if you are building rockets etc. 🙂) and use it more broadly from users perspective, while also reviewing the code in pairs while talking etc.
As he said -- this is probably one of the only ways to ensure that the changes work with each other multiple times per day. I find this also allows for people to work simultaneously on same / overlapping issues, which is always a hassle with gitflow
He’s suggesting pair programming as the replacement for code reviews. Essentially, the devs are reviewing themselves.
@DontDoIt I got that he thinks pair programming is the best way of doing code review, not replacement.
Having the person there explaining the changes is extremely helpful when reviewing
@@Blob64bit but if the code reviews are not a gate, it doesn't have to be reviewed at all before it hits production, which is the part I don't like. This just means it doesn't have to be reviewed at all. You have to assume that developers, including junior developers, never make mistakes that aren't caught by their own unit tests, but sometimes there are misunderstandings, or badly written tests etc. You also need to make sure that you do the reviews later and come back to improve the code, but this is just adding a lot of tech debt, and if you can't repay it fast enough, tech debt slows you down. Also, pressure for features often means that going back and improving things after delivering them is seen as a low priority by the business. As far as they care you delivered and now it's time to move on to the next thing.
@@dontdoit6986 he starts by saying he usually recommends pair programming (which I'm fine with) but then goes on to suggest an alternative approach in not blocking the release with code reviews, which I don't like as much, because it means code might not be reviewed at all before it hits production.
1 minute in to the video, and really curious what comes now. what a hook! Well done Dave!
Honestly, this is the best channel of the kind.
"We don't improve quality by adding bureaucracy" that should go on a T-Shirt!
Thanks Dave. I am borderline autistic so pair programming is not an option for me. The cognitive load of coping with someone else in my space overwhelms me, so Thierry's solution is an excellent alternative.
I used to do the equivalent of pair programming in electronics engineering where I sat with a PCB CAD engineer and we worked on circuit board layouts together. The layouts came out great, but colleagues that did not do this had poor results with tracks burning up and bad component placement etc.
Feature branches should be rebased each time any branch is merged back to trunk. This ensures that all changes are integrated each time any change is ready to be pulled in. This branching and merging strategy integrates code as fast as it's merged, and as fast as any CI strategy. The thing which many people don't take into account is that there is inherent risk to pushing directly to trunk for the entire team, and time IS lost in editing the DAG to correct pushed mistakes.
This always comes up. If you do this and you have two feature branches created today, and neither of them get merged for a week then they are not integrated with each other for that week. I could delete a class in one branch and you could add properties to it in the other branch. Since neither of them is being merged the issue won't necassarily be detected. It would be worse if it was a more subtle incompatibility.
@@barneylaurance1865 Potential merge conflicts can be solved by timely communication between the involved team members.
Confuse correlation with causation. the quality gates might be there, because the quality has been bad in the first place.
That is always a difficult problem in sociology. Hard to distinguish, but this does sound a bit like you are dissing the methodology, maybe by guessing what the methodology was? That isn't good science either. Have you read the book that describes the approach?
@@ContinuousDelivery you're right, in that I'm just quoting you from the introduction without going through your sources. Which data are you referring to? The state of devops report?
BTW I completely agree with your own methodology and conclusion, combining data and explanatory models. Just stumbled over the correlation part.
I'm into the topic because I try to estimate the effect of pair programming in one of my own projects but I think it's confounded by the fact that only easy stuff is done alone - thus outperforming pairs in some way 😅
I'm in a team using Kanban and Pull Requests for reviews. We have status meetings every morning to check the board. 30 minutes. Mostly to make sure that the throughput is constant - following the WIP limits. We constantly struggle with respecting them - since when work becomes a PR in the Review column the developer is eagerly looking for something else to do. I don't like this stress och pushing things through and feeling bad when stuff aren't moving on the board. That ignores the joy of software development.
Sounds like a bit of broken KanBan. Start by removing the "review" column and only have "doing" and "done" columns . Then set a WIP limit in doing.
That way no one can start something new unless something moves out of "doing" and the only way to do that is to finish it. Then maybe the CI bit will start to click...
@@ddanielsandberg Or better yet, patch the Kanban software to not allow pulling from "to do" unless "review" is empty.
This must be a very common issue since I've also seen 2 projects like this. A lot of pressure and not productive at all!
Sounds like the objective on that project is to push items from left to right on a Kanban board, not make better software, faster. Kanban is a very good tool (used it for years) if you understand its objective.
@@ForgottenKnight1 OK! Great!
I think some pick Kanban because it is simpler than "Agile" methodologies. Easier to explain what happens, what they supposedly get from it, and get everyone on-board. Leaves little room for complaints.
The problem arises when you don't value quality as much as getting stuff done. Or when everything gets reduced to the item on the board, rather than what it is about.
I dislike when someone talks about something as "just an item" in the backlog or the board.
Where I work I try to encourage pair programming and any code that is pair programmed automatically passes code review and moves past the review column in the Kanban board. While code that is not pair programmed is gate kept on a feature branch. I'm trying to move towards more and more pair programming.
The issue I see with this is code reviewing an entire feature of small commits with other commits in between the ones I'm interested in would be difficult and it'd be hard to track their changes over time to know if they are good or not. Something may look bad only to get fixed in a later commit or look good and get worse in a later commit. Or someone else changed the class in between their commits. I've found that having any kind of code review tends to implicitly encourage larger commits as they are easier to review from looking at the git diffs.
On a somewhat unrelated note. We have a very elementary pipeline set up and our unit tests take just a few seconds while our acceptance tests take around 15~20 minutes. Because of this I rarely run the acceptance tests on my machine or while pair programming and am nervous to commit to trunk directly as unfortunately we frequently accidentally break the acceptance tests due to oversights. Any advice on how to handle this? At the moment I commit to a feature branch, wait for it to pass and then merge to trunk later in the day.
I'm not an expert, but from what I understood you would accept the risk of potentially breaking trunk, but need to get build notifications and fix any issues that come up asap with highest priority to ensure it doesn't stay broken. And work to make the test duration shorter I guess.
You could use a method from lean manufacturing. I think it was called the „andon cord“.
Translated to the world of software engineering, it would mean:
Make the state of the deployment pipeline visible to everyone in the team. (In the old „office times“, we had a screen in the office of our team showing the dashboard of our CD tool which was constantly refreshing - now I have a live version of the dashboard open on an external monitor or getting automated slack messages for status updates).
If the deployment pipeline on the main branch turns red, you immediately coordinate how to fix it (mostly the author of the commit will take this or you pair/mob to resolve the issue).
In the mean time the team could also work on „prioritizing“ the acceptance tests so that there is a suite of „critical“ tests that can be executed on the local machine in reasonable time and run all the tests including the ones for edge cases (or lower priority things) later in the pipeline.
You could also try scoping the tests „by feature“ so that you can run acceptance tests of the things you’ve most likely touched on your machine in less time and run all the tests in the pipeline once the change is integrated.
It is so frustrating when we have the goal of CD, but also have to use pull requests and code review...
"...have to use..." - who's forcing you ?
Limitations of pairing:
- time zones (distributed team)
- language barrier (team mates don’t share the same primary language)
- cultural differences, many cultures have a completely different work ethic and culture.
It looks like you are listing limitations for communication, not pairing.
Exactly. Sounds like the communication is the underlying issue here.
+ Budget (aka, not allowed to hire) is also a limitation.
+ Overcoming the perception that one person in the pair is "not working"
@@ApprendreSansNecessite Communication is a limitation for pairing
In that case, you can do pair programming 2,3 hours a day (you're lieing to yourself if you say you'll do it 8 hours a day)
Language barrier - this seems to me like a communication issue, so improve that over time.
Cultural differences - same as language.
I've worked in aerospace for 8 years now on greenfield and legacy projects and I'll add my two cents. In my opinion, reviewing the requirements and tests before writing and committing code is absolutely critical. As others have mentioned, I don't think tests should be merged to a release branch without a formal review, whether it's a pull/merge request, or everyone sits in a room and goes through things step by step before the changes are pushed to the trunk branch. Adhering to BDD and TDD principles by specifying a feature and the test scenarios that describe it/prove it works as intended, then reviewing with all of the stakeholders should be done before any code is committed because ambiguity will be minimized for all stakeholders. For example, someone without knowledge of the domain or hardware systems the software is controlling may think some utility command is irrelevant and exclude it from the implementation while the operations engineer using the software assumes they'll be able to send that command because the component specification the software will interface with says the command exists. Once that baseline is established and agreed upon, then I agree with everything else you described here provided there is automation in place that couples/traces the execution of the tests back to the same requirements and test scenario artifacts that were previously reviewed and approved by the stakeholders, and that the trunk branch cannot be automatically deployed to a production environment without a final deployment readiness review. Of course, feature/test deltas need to be reviewed just as the baseline. Too often I have experienced the exact issues you describe which lead to unnecessary delays, increased cost, and poor quality software. When you have a seemingly limitless amount of capital and personnel to work with, the old Apollo method of developing software looks and sounds great, especially when the outputs of the process are good enough", but it certainly doesn't optimize for cost and isn't benefiting small space start-ups in these less economically forgiving times. Thanks for producing these videos Dave, extremely insightful stuff.
Honestly, for aerospace, you need to take it slow.
The Pair Programming and Agile stuff is suited for non-mission critical software you find in regular corporations. IT/data stuff. Don’t follow this advice for firmware, especially safety-critical firmware.
@@mecanuktutorials6476 Seems more like QA testing issue here more than a code-review issue. No matter how meticulous and thorough the code-review and automated testing process is, only manual QA testing can reliably determine if software is production-ready. Even 100% correctly written code doesn't guarantee software will work as intended. This goes _double_ for mission critical components. And while code-review can be a beneficial preventative measure to avoid potential issues early, when the process becomes too bloated it serves as red-tape that prevents getting timely feedback from proper QA testing. A rapid develop-QA test-rework feedback loop is arguably more valuable than extremely detailed code review. They are not mutually exclusive practices however. Ideally, they should complement each other. Unfortunately, many companies tend to double down on the red-tape aspect when things go wrong, trying to lockdown the process to prevent any and all mistakes within the code, instead of streamlining the process to get feedback quickly as possible.
@@philw3039 I agree.
Only manual testing can ever really proxy what the user will experience.
In my experience, firmware development is closer to mechanical manufacturing in how it is handled. There is a stark difference between it and Desktop/Web/Mobile which are intended to run on multiple abstracted platforms. A lot of these other fields can “trust” the layer below works and covers ur behind. That trust extends to reviewing other’s work and lets things like Pair Programming happen. You only talk at a high level because the lower level stuff is trusted to work. But in firmware, things have to be more precise. Firmware is the kind of code that really does converge to a requirement. It’s usecase-specific and too costly to shift gears because it is so tied to the system/hardware. Code review is the only way to review details independently and provide precise detailed feedback or concerns.
Code review during any development is indeed a hurdle because you’re just trying to make it function. Too much feedback and back-forths become an obstruction to further development. At some point though, the work has to be made presentable for others to read and maintain as well as give others an eye on the state of the codebase.
Overall, pair programming or code reviews are not one size fits all tools. Trying to enforce them in all situations will diminish their value. But manual testing is inevitable.
Wow, nice to hear this idea. I just started implementing TBD since December and I found great value to keep a Code Review step but stil do CI. I'm so glad to hear this idea from you ❤
The thing about dogma is that a lot of the time people don't realize that they're following it, they're just doing what "seems obvious".
Huh, I had a similar idea a couple of months ago. The idea that we don't need to consider our software "releasable" only after it is reviewed and tested.
We could trust our team members that they can write code that works. Using TDD, API-first design and such. It's just that the code might not be of the best quality it has a potential to. That's why reviews could've been done asynchronously or in a pair after the feature is working.
Unfortunately, I haven't been able to try this idea in full, since I'm the only backend engineer in my projects. Plus, I didn't have enough courage to suggest making such a "radical" experiment.
I agree with others who might wish to share this sentiment - the channel's weekly videos are lengthy and standalone, making it tough to keep up. Instead of eagerly waiting for Dave's content, I find myself questioning their value (even considering if they're worth my lunchtime). The view count compared to the subscriber base speaks volumes.
I propose less complex but still insightful content, with more thematic connections for binge-watching. Avoid churning out filler content to meet a quota. A lesson learned from your own Cyberpunk 2077 video about managing time and scope. One value quality over quantity, a good video is more appreciated than weekly releases only you yourself seem to value.
Thanks anyway for the effort, team Dave, loved the book.
I agree with getting code out into prod asap to get feedback, but I’m still still not convinced on the No branch policy. Firstly, just pulling from remote creates a distinct local branch. But more than that, I’m usually working on a few ideas at once, swapping between them over a day. Or checking out other changes from other team members… I don’t know how you could do this without branches…. Maybe with git worktrees… but it’s kinda like working with another branch anyways… further more, doing this my team is still able to deploy multiple times a day… and after, we just rebase and continue. But granted, I don’t know how to get rid of the PR without the equivalent of pair programming before release. But do we need to review in person? Sometimes it’s definitively better, especially for complex changes.
Easy-peasy. The solution to waiting for feature review is not review the feature, but split feature into small tasks which could be reviewed in minutes. I do. No "ready for review" task state, just a message to the "Mearge Request" chat is required. If I'm not on the meeting then response will be in 5 minutes. If I'm on meeting and not currently speaking than the same. Another point that I count releasable any code that compliled and deployed successfuly. Trunk base development with feature switching is expensive, we just open a button or api when the whole feature is ready.
Hi Dave, I do resonate with a lot of what you said for small closed teams and private development, and I do see a lot I personally can improve that can lower frustration and increase value.
Now for the elephant in the room, it's not the same for Open Source projects, and currently validation has to be done in the form of pull requests, So things tend to be slow for outsiders of the core team to add code to the project, even high quality code.
I don't think there is really a better alternative to that but I would love a video of yours commenting on it.
Thanks for the videos.
I love this idea! I'll see if I can try it on an actual project. Best of two worlds.
Fantastic timing on this video! I'm currently acting as an enterprise architect for a very large and well-known company (I'm a consultant for an equally famous global company) and just this morning we were discussing code review and branching strategies for a specific application. I shared a link to one of your older videos on this topic. I can use this new video immediately as a discussion point. I agree with you completely and I'm glad you see other legit approaches in addition to pair programming. I'm working hard to develop an idea that bridges the gap between true pairing and non-blocking code reviews as at very large and older companies facilitating swift change on your personal timeline is a fool's errand.
15:35 how is code “code hidden away on a feature branch” any different from code checked out locally and not pushed to the remote trunk?
Because we never hold on to changes for more than 10-15 minutes.
Yes! We don’t need to follow the same model used on large open source projects.
But need at least to understand that the PR model comes to solve exactly the problems that Trunk base development brings to the table.
Seriously I was looking for a take on code review from you
What data are you talking about when you say process gates correlate with poorer quality? Can you reference the study since it's one of the key axioms of the video?
I just need to say that obvious thing: LOVE YOUR T-SHIRT!!! It's awesome
Skip to 16:30 to save your time. That's where the alternative approach starts
... and then you miss the real lesson 😉😁
Claims to have something better than PRs, 2/3 f the way through the video still nothing but talk about how bad PRs are... How about getting to the point so we can shorten the feedback loop on this video?
I recently worked in a project that had a design problem which I was trying to fix. But because of the pull-request & review based release method I had to merge the re-designed code three times, and wait on a review each time. This added four weeks to the release. And made me decide to leave the project...
14:39 hold on… the branches should be rebased, and so have all the latest changes…
I'll paste my previous answer... for the 27315th time:
"Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches?
None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange. What is strange is the obsession with branches to isolate changes and hide systemic issues."
Doing something new... is often a topic of courage to take a first step for trying it out.
12:31 Wait, what? Don't most people regularly merge in the main branch (whatever it's called) into their feature branch, like I do?
I'll paste my previous answer... for the 27316th time:
"Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches?
None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange."
Instead of claiming that data shows that pull requests affect quality negatively, how about actually backing up that claim with actual evidence? It is interesting, because Google is also utilizing something like pull requests and they have made internal studies into the effectivness of code reviews and many aspects of code reviews and their internal studies seem to suggest that it absolutely makes sense (as stated in the book 'Software Engineering at Google'). How come that Google came up with a completely different conclusion than whatever data you have?
Well, There are links in the notes on this video to the data that backs these claims and I don't know which data from Google that you are referring to, Google is a big org, but my primary source is the data from the DORA metrics, which is now owned and managed by Google! This is based on the most scientifically valid study of software development practice that I know of, and on the results of 10's of thousands of survey responses.You can read about all this and the reason to trust this data more than anecdote, in the Accelerate book.
@@ContinuousDelivery I have looked through the links in the description, none of the links provides any data about pull request reviews. Which link do you mean? As for DORA data, that's again very vague. Which DORA data point to pair programming better than code reviews? Under what settings? I don't know where to look for the data that you claim you have. I wasn't able to find any data of pair programming vs pull request on the internet, where were you able to find that data?
this sound great, I can already see the face of disagreement from the "white collar" part of the team
Dave, could you adress how you would "solve" pair programming in a distributed (time and/or space) team, that for many good reasons agreed on asynchronous work? TY
Hey, Dave!
I appreciate your videos and have read your book! I have found both to be very helpful! But I am hoping to get some advice on how do we get the team to that level.
How can we get the team to be comfortable with the approach you propose in this video?
Currently, my team follows a gitflow and pull request process. Exactly as you mention it in this video. The team is young. Almost everyone has 5-8 years professional experience. Everyone is aware of what a CD process should look like but none want to 'risk' following a new process they do not understand. How can I get them more comfortable with it? Keep in mind that I cannot force them to read your book and watch your videos 😅. As far as I know, convincing your team is a whole different beast.
Maybe you could try a Ship-Show-Ask approach as a stepping stone.
„Everyone is aware of what a CD process should look like“
Then the work of convincing might be already done if the team agrees on roughly the same CD process and that they also want to implement this (this would be the first question - ask what they imagine to be sure CD is understood by everyone).
Changing to CD is not done over night. So maybe discuss some small steps you could take as a team from the current state to move a bit closer to the desired state or process and investigate from there.
From there you could improve further or roll back and try a different direction (as it is in agile product development).
This might overcome the fear of trying something entirely new.
The team might need some help from others (or management) to fulfill some prerequisites.
For example the team would need the autonomy to deploy their software independently at some point. - This means the software needs to be testable and deployable independently. - This means the team needs to have the full authority to determine if the software is releasable or not. … and so on.
You might need to convince a tech leader in the company to follow these kinds of goals (hopefully you don’t have to convince for these goals but my experience tells a different story 😅).
So do changes in small steps and you’ll get there at some time. If you need help from a person with more experience in this way of working, asking questions (or for help) about these tiny improvements will also give a more detailed answer.
Hey Dave,
right at the beginning you refer to some kind of data that states the correlation between process gaps and quality. I'd love to see that data. Could you give a hint where you found that?
Thanks for your awesome content!
Also trunk development would make the team members start to own the whole codebase and not only their own branch. But yeah, that requires maturity from the members. But maturity comes with responsibility. So, kind of a chicken-egg problem :)
Regarding the situation where you need to roll back changes for the feature, and imagine, you have several team members contribute their parts to the feature, like backend, frontend and e2e tests, you may get into troubles, having to search for every single commit related to the feature, and removing it. And if those commits contained any unrelated changes, this may break the product.
To overcome this, you then also need to have a rule like, that feature releated commits are clearly separated from any other code improvements you want to make to the code.
Well, again, that requires high maturity and sense of responsiblity from the team members.
But yeah, i can confirm, the merging hell with feature branches is real.
Another scenario, how do you make breaking experimental changes with trunk based development? I see no other solution than experimental branch, but that would be much less frequent event, than merging hell on a regular basis.
yeah, you're actually bringing up a real life case vs. the ideal ones that Dave used to showcase his ideas.
I mean, on a living system, when you need to change a feature (and if it is also a dependency for others) what do you do? I think i will anserw myself, that one needs to actually create a new feature, not change the old one, then use feature flags to switch one for the other when ready. still, this is most often hard to sell as everyone sees such aporoach as more effort 🤷
It will be awesome to know your opinion about what I think is the biggest issue happening to programming nowadays. What do you think about the common practice of letting Business Analysts and Stakeholders design software? In every project I worked (except for the small ones) Business Analysts and Stakeholders design the application till the most tiny detail and that has a lot of impact on how you implement that solution. A small detail in the UI can impact even your DB Model, in the end people without any technical knowledge is designing the architecture of your system. Is very common to see UX designers closer to the Business team rather than the development team, and that is awful. Also this practice is taking all the fun out of being a software engineer.
Can you describe a bit more that UX designer situation? How are they closer to the business and what are the consequences?
@@ApprendreSansNecessite In my case, UX/UI designers present the mockups to the business before presenting them to the dev team, and there is a lot of bureaucracy required to ask for a change
@@leonardomangano6861 how can the business force you to couple the database and the UI? What lead to this? I am not questioning your testimonial, it is just so odd to me.
@@ApprendreSansNecessite It's not that the business forced me to couple them. The UI and the database model are coupled by definition. The design choices you make in the UI will impact your DB model
@@leonardomangano6861 UIs and databases change at different rates and for different reasons. You cannot blame the business or the UX designer for having failed to put an architectural boundary there.
I've tried Ship-Show-Ask in one project and it worked quite well with the experienced team.
With "Ship", there are no code reviews at all, is that right? Cause then, in this aspect that could be even more "radical" than what is described in the video (where some kind of code review always happens at some point if I understood correctly), depending on what kind of changes the team decides can be "Shipped" and what should be "Asked"/"Shown".
Pair programming is expensive. Up to x2 the time (and developer salaries) will be spent on developing the same functionality
Similar thing can be said about TDD. It doesn’t always make sense spending a lot of time on covering all of the functionality with tests. Sometimes the cost of error is low. And it would be much cheaper and effective to get the feedback from user experiencing an error or see it in production or staging sentry (like log but error aggregator)
I hate when people push pair programming. My small team is spread between west coast US, east coast US, and Eastern Europe. How the heck am I supposed to shoehorn that in?
I have done this, pair in the periods when the timezone overlaps.
Question: you say there is a negative correlation between proces gates (like pull requests) and quality. Has this correlation been controlled for other factors like project size?
I was wondering this myself: does the level of experience for example go hand-in-hand with the felt need for extra gates? Or some other correlation.
Overall there is a deeper survivorship-bias in the underlying statistical analysis of fewer "blocking" quality gates (like PR) leading to higher quality / delivery performance. Accelerate does go a little into this when analyzing the DORA data, but the focus is on other parts.
Still, it should be understood that highly efficient / capable teams should not get blocked by such gates. But there are also less capable teams, or inexperienced ones. Or ones that only start to form skills and trust. In those case removing most gates is dangerous.
But we do not get to see any data from such teams, because the company will either hire consultants to fix the damage or go out of business. Therefore we do not see any data from those failing teams in the data.
To deal with this bias it is better to start with great teams and get rid of the training wheels one at a time. Given enough time, skills and trust within the team will increase enough for them to gauge quality their own way. That is when to switch to Trunk-based development, not before.
The extreme worst case in comparison: a majority of the team is both new and unlikely to stay for more than a few months. Do you really want to switch to asynchronous quality checking of their code (which the video is basically about) if they're unlikely to care for any bad consequences or damage to their reputation?
And if you actually weigh by teams instead of projects, this kinds of fundamentally broken teams are just way more likely to happen because highly efficient teams are so efficient they can do a lot more projects within the same time. So if projects follow a normal distribution overall in the industry, there must be more bad teams than good ones because bad teams not only have worse projects, but also deliver fewer successfully.
So please do be wary of this bias overall.
14:12 Before merging feature to develop, develop branch with verified changes of other developers should be merged to the feature branch. After this CI verify all verified changes of other developers plus not yet verified changes of the feature branch. There are the settings in git providers that doesn’t allow to merge from outdated feature branch.
With Home Office and less joint working time, code review or pair programming becomes really hard to establish. How to deal with that?
I trust everything this man says. Thanks
Careful now!
That sounds like a dogma
Just kidding...I like to think after his videos
How would you advise to work in an environment such as a game development team where some people are working on a game and others are working on engine features or lower level features of gameplay?
having worked with a CI oriented team in such environment, it could be challenging to have parts of the engine broken for several days when you were developing a gameplay feature... How to accommodate game designers that need a kind of "LTS" version of the tools they use with the need of continuously testing and breaking things of the engineers building the tools? Would you say CI is inadapted to game development?
Well, sorry, but it isn't CI if there are "parts of the engine broken for days"!
The aim is to make small changes that keep the SW working after every small change, commit them to check that these small changes work with everyone else's small changes and if one of your changes breaks something at that point, you fix it to revert the change immediately.
CI is about committing WORKING SW multiple times per day, it is a more incremental way of working. CI and CD both work very well for game development. It takes a bit of adaption in the way that you work, and it sounds like the team you worked with hadn't done that.
Of course your Kanban board should have queue columns - that's a given for the pull system and absolutely not mutually exclusive of PRs.
This video is just the same 'ol recycled contortions propping-up anti-PR dogma. PRs can or can not be trunk-based development. PRs can or can not be CI. Its like any other tool and depends on the use/implementation. Saying PRs can not be trunk-base or can not be CI because a few cherry-picked anecdotes bites the Dogma disclaimer at the beginning that was intended to preemptively claim this anti-PR dogma is not dogma.
Feels a lot like FUD to sale a training course and/or youtube ads (and yes, i got suckered thinking there would actually be some new info). If not FUD or bait, lets disambiguate. How much time can code be non-integrated for it to still count at CI? is one day? how about 1/2? Share the bullet points of the alternative that "hides" code materially less than what is possible with a PR.
The process I like, that uses PRs , is auto merged to main on green and since it is fully tested and deployed outside of main, including with main's code, there is never the waste of a roll back. Only when someone has doing some thing "creative" with git is there ever a merge conflict and since the conflict is in the PR's merge branch, it keeps main in a deploy-able state.
Many thanks for your materials!
Thank you for inspiring us to taking our processes to the next level!
As you suggest, we also switched to trunk based development completely and we are happy with it so far.
As a consequence, we do have a question about deploy version management: If our CI/CD server now creates a version bump automatically, we would need to also commit this change to VCS, essentially creating another automatic commit for each pushed commit. While this does work, it does not seem to me to be the best practice. I'd be curious to hear your thoughts / opinion. 😄
I am pleased, but not surprised, that it is working for you 😊
There is no perfect answer to the version number problem, other than discarding 'semantic versioning' which isn't a great answer either, because it is useful to communicate with customers and users.
My recommendation is use a generated unique id as the 'real version number' or 'release-candidate id' and treat the semantic version as a kind of 'display name'. You can keep a record associated the display name to the correct version number somewhere else and so not compromise the VCS of the release candidate. The problem with this is when you need to package the display name as part of a binary delivery. It is largely not a hugely risky thing to add it as a final packaging step, but I prefer the purity of 'test what you deploy into prod' if you can manage it. So a compromise, but not a horribly risky one.
@@ContinuousDelivery Amazing! Thank you Dave for the detailed response. 🙏🏽
(I'm in the same team as @coZhenya.)
I love the idea of treating the "user"-needs (version number display) and "tech"-needs (unique-id) separately!
I admire the consistency of your applied mindset and context-aware openness that I see reflected in your reply.
=> Why only use the single responsibility principle in code. 👏🏽
And yes, you anticipated correctly that I want to inject the version(s) into the build so users can supply that information for debugging purposes.
I concur with your preference of bumping/generating the version(s) prior to testing to keep it pure/clean.
Since I'm rather new to trunk only development: Would you be so kind to elaborate on your thoughts about the double-commit challenge?
Is it perfectly normal to let the ci/cd pipeline commit the changes so that a future release is aware of the last version?
Or was the idea that using a generated id replaces the need to "remember" the previous (non-user-dispaly) version?
Thanks for sharing your intelligence about the ability to change. ☺💙
@@coDocumentary My preference is to use the "real release candidate id" and only associate the "semantic-version-display-name" once the pipeline has approved the change. So I have done this, in the past, with a separate step and separate, tiny, stage that 'attaches' the version to the release-candidate. In one version we patched a file, designed for the purpose, with the display name version number. It depends too much on you tech to go much beyond that as advice.
I think what you are looking for is the minimally intrusive way to 'attach' the display name to the code.
@@ContinuousDelivery Perfect. I appreciate your advice and it is enough for me to get the idea. I was a bit hesitant with patching the code at that point, however your strategy and the reasoning behind it does make sense to me and I feel more confident to commit to testing/adopting that approach now. 🥰
Pull requests help track the pieces of code to review and make it possible to comment and all that jazz. Do you have suggestions for an alternate flow? The review column is what I would reach for instinctively and I guess semantic commits help a lot but doesn't it feel a bit messy / inconvenient?
Tools like Jira/Bitbucket will cross-reference commits to tasks. If it is the task that is progressing into a needing-review state then you can find the relevant commits directly from the task.
@@monarodan This is the way.
@@monarodan This is also expensive, but I guess it depends on your situation
@@ApprendreSansNecessite why do you say it's expensive? It's literally just mentioning the ticket id in your commit message.
@@monarodan my bad, I was looking at the wrong number of licences when checking Jira/Bitbucket pricing.
strongest issue with this assesment is that developers are more often than not: inexperienced, unskilled, messy, chaotic, conceited and arrogant. It's not that it's intentional but developers' ego are something that is making things like CI/CD is effectively impossible.
I worked at a large corp who's main reviewer unironically would block PR's because fields weren't in alphabetical order. It was fucking hilarious!
Dave, I love community comments and polemics under your content. Maybe it is a matter of not being a native speaker or but I understand and learn more from comments than from the actual presentation (visual side is great, but maybe sophisticated speach strictire or presented concepts are too general aka high level for me, a simple dev?)
Post Merge code reviews For The Win!
Loving the subtle homestuck shirt
So if you don't use feature branches, that means that every commit is deployable? How do you commit early and often ?
Another problem I can see is people. Some may just feel uncomfortable knowing their code can go to production without a code review. The process allows to account for that by doing on demand pair programming of course, but this just goes to show that it is not suitable for everyone. The team has to be fairly mature I think.
I always wonder, and can't quite figure it out, is it possible to have different people write tests for your code and still do CI ? If someone else is going to write, let's say, integration tests, then there's always some code in trunk that has not been tested yet (but all tests pass because tests for new code don't exist yet). So then you can never trust your CI, because you never know if tests for all new code have been added yet. And even if you knew that, testing is probably always a bit behind development, so you can never release.
It's always so cool to hear all these great approaches to getting quicker feedback and learn from each-other in the workplace. But I sometimes wonder how this will work in a 2 people team or a team of just-starting-juniors and only a few seniors or only one or none. Won't it be nearly impossible in these situations?
Pair programming by rotation. Depending on the size of your team, you might have to do multiple permutations before all seniors have worked with all juniors. There are plenty of videos on YT on how to do this.
Asynchronous Pair Programming!
Pair programming and TDD are being rejected for some reason. I wonder why? Maybe because I don't have a bunch of Kent Becks working for me. If I had, I wouldn't need pull requests and code reviews either.
Maybe, but another way of looking at it, is how do we create the next generation of Kent Becks to work on our team, I'd say through Pair Programming and TDD.
@@ContinuousDelivery I can't agree more. Extreme Programming Explained is a great book. However, the programmers I work with don't have skills to pull off TDD. And if you put two juniors to pair program, you may end up getting the double spaghetti code 🙂 Just yesterday was reviewing code. Test passed, however the way the code structured there was a potential for concurrency issue. Had we released it in production we would have errors happening once in a blue moon and we wouldn't know why. Reality of business and scarce resources forces you to work with what you have.
How many times have I carried out a pull request code review and the code is not how I would want it but it works and re-writing would be too costly. Pair programming would have caught this at the earliest stage and there's your quality built in.
$0.02/wo: The PR seems to have come from organizations that have a particular management style and culture (Google?, and perhaps Linus) where the idea is that "you are not worthy" to commit code to "my repo". So you are expected to doff your cap, bow, and present your miserable code for blessing by the priests. I'm sure this works fine in that culture. But most of us don't have that culture. We hire developers that we expect to be competent. We fire them if they're not competent. So the whole notion of bowing and scraping to be allowed to commit code is not applicable. Instead we might want to have a manager or dev lead _look_ at code being committed in order to catch bad code and maintain their list of people to fire or send for additional training, but there's no need for a review-delay-gate. We might also want to have a formal code review process in some circumstances, or for some kinds of code (e.g. security-related). I think this is roughly what Dave is describing in this video. The PR isn't designed to facilitate that, but there is a need for some tooling support -- a kind of "code queued for review" feature in the sccs -- otherwise how would we know what code had changed in the development of a particular feature, after the fact?
I agree with the core of your point, perhaps not the cause, I think PRs come from the open source world, where it is sensible to be a bit wary of unknown people committing to the core of a widely-used operating system (for example).
@@ContinuousDelivery Fair point, but I think also used as an excuse to justify assish behavior.
We do this but with a set of different rules. All code goes on to trunk reviewed. Changes should be small, not nessescarily a whole feature. The code that goes in is releasable meaining the release artifact works. Only thing that blocks the release should be code design changes and code that doesn't work. Everything else can be fixed later. This approach works quite well.
I know pair programming is better but the human side of pair programming makes it too unpredictable for me. Probably if you do it disciplined enough it would work but I just can't concentrate if someone is looking over my shoulder. I guess if you have a good process like ping-pong it will help it but it takes too much training for the other guy to stick to it.
what you rebase your branch periodically to keep up with trunk?
I agree Dave, but our team sucks too much to practice real CI
Wait for the next video!… 😉
@@ContinuousDelivery I can't wait for this video, because my team sucks too.
PR's has no direct relation to quality, but to two other reasons.
If you are using PR/MR for quality, then you are doing it wrong.
The reasons are, first, the code been ready this happens because you need the team to be able to develop things without changing the behaviour of the mother branch, until you have finished your work, allowing that the system could be validated in its isolation(No other features intervention).
The second one is related to the fact that not every developer is Senior. Companies have a lot of people on different maturity levels working on the code, so you need to review to make sure that code directives and patterns are been followed.
They have a measurably negative effect on quality if they slow the feedback process (CI).
You other recommended uses for PRs are also better fulfilled by other ways of organising code review, pair programming and continuous-review (described in this video).
Finally on your final point PRs as a teaching mechanism do have a place, but are very poor second compared to pair programming.
Who is Thierry and where can I learn more?
There is a link in the description to this video, but also here: bit.ly/3JiJUxV
Maybe I'm too tired already, but I scrolled through the video description several times and couldn't find the link.
My Question about this is what do you do with unfinished features tho? Releasing unused code into production is a pattern I thought was considered bad?
Depends who you ask I suppose, and what you mean by "unfinished". There are ways of working that make this safer than the alternatives, this is how SpaceX work for example. The key is that unfinished does NOT mean un-tested. All the code released will be tested and working as designed, though it may not yet, add up in some parts to a finished feature.
A couple of things stand out:
It seems like you're referring to human-gated pull requests. I'm not sure that pull requests need to involve a human. You could have a series of fast automated checks and, upon passing those checks, merge the code. Some examples of checks would be verifying no linter configuration changes, running your linter/formatter, running static analysis and having no new (major) potential vulnerabilities, running automated tests and passing, no newly disabled automated tests, changes introduced have line coverage.
This could also give you a good list of things to review. If your tooling allows you to find merged pull requests that have no human approvals, you can review the change. There's always a chance that future changes could change the context of the pull request, though. Minimally, it points you to the work done that hasn't been looked at by a human.
I build all of the things that you mention into the commit stage of my deployment pipelines, this is CI not really a PR, and I'd argue that PRs don't really add anything at that point. Why bother creating a branch, when you already have one in the form of 'my-local/main' and you can trigger all the evaluations that you mention on push to 'origin/main'?
@@ContinuousDelivery I suppose if you have tools that allow you to use hooks, you can run the checks and rollback the merge. Not all repository hosting services allow for server-side hooks, though. It just feels cleaner and safer to have a remote server with controlled configurations run these checks to avoid tampering. Someone independent from the development team can control the configuration of the tooling used to execute and approve the automated PR checks to ensure that the checks are in place and can't be overridden by the developer of a change. It lets code get into the mainline faster. I'd rather make sure that all the fast required checks pass before integrating and defer some of the more intensive checks to post-integration (like a more robust suite of automated tests, dynamic application scanning, and so on).
That isn't to say that I wouldn't rather do pair/ensemble programming and have a robust CD pipeline. This could be more in risk mitigations for cases where you need more human monitoring or involvement in the process.
You don't really need VCS support for this, CI systems will do the job too.
Anodyne bureaucratic box-ticking code reviews FTW.
Why not just merge trunk into your branch every day?
Because it doesn't work. Let's say you have 20 developers, and that each have their own feature branch. Even if all of them rebases from master every hour - how many changes are integrated to the other branches?
None! They will stay continuously isolated and you won't know which branches that conflicts with which other branches until one of the conflicting branches is merged into main. CI/TBD is about exposing conflicts early by continuously integrating into mainline. It's nothing strange. What is strange is the obsession with branches to isolate changes and hide systemic issues.
@ddanielsandberg
Feature branches should be merged in at minimum once per day. Thus, everyone's changes are merged and conflicts are noticed. It's pretty straightforward.
So this model doesnt allow for bugs because we have tests, but if I write the code and the tests, then any bugs im not aware of will be in both. We make sure reviews are done by separate people but the tests (that we rely on) aren't held to the same standard?
No, there will still be bugs, there is data to say that there will be about 60% few bugs in production if you adopt a good testing approach, there is no data that I am aware of that indicates that PRs reduce bug count, and in fact when compared to more flow based practices, like CI, then the data is very clear, and quoted in the video, based on the DORA metrics of Stability & Throughput, teams that practice CI score a lot better on both stability (a measure of quality) and throughput (a measure of efficiency).
If you practice TDD, which is my preference, you are less likely to have bugs in tests, because the process includes a validation of the test - we always run the test first, before we have created the code to make the test pass, so that we can see that the test is actually failing as we expect.
I will never allow unreviewed code into the open. The moment you are dealing with anything important it has to be seen.
If you write small commits for big things sometimes you lose context.
I see why ci cd dislike feature branches but this system seem like it introduces entropy and buried errors.
Love the shirt 😊
Thanks! But you should work on the slides, its not needed to see you on full screen all the time, its more important to see diagrams and text (for longer periods of time) that explains the main points you are saying. As it is now its hard to understand your point.
Hi Dave, I agree to your arguments but there are cons you do not talk about. Trunk based development fails if your collegues are Not trust-worthy. It gets a big mess if the work is not done as expected. Lets say the developer does not write tests and adds buggy code. It gets even more evil, if they act like to sabotage the work.
In those cases you need feature branches to limit the damage to that Branch.
Somewhere in future when the team is formed you can move back to trunk based. It would be nice, If you can talk about this more often because in reality, it takes so much energy to get your team to that sweet spot of CI you are talking about.
Why work in company where you cannot trust your colleques?
@@Blob64bit My personal opinion: Running is no solution for me. I use just use different tactics on different situations. The goal is get the team in shape. I had success so far. If the team is in one level, you can get back to what Dave said. And you can be proud on your team. Anyways, you have to deal with all kind of people, even those that add negative value.
I think too many assumptions made here. We use pull requests to scope the change(s), which are automatically linked to the issue for documention for the future. Nobody said pull request approval has to be performed by a human. We have more than one approver for a pull request and non of them are human and they are all quality related. Sometimes you can sync to a trunk way too soon which massively adds to work. ...and so on and so on... The assumptions about how people use pull requests are too many.
This video asusmes that the developers in your tram are good. Around 80% of devs i worked with for 20 years have been mediocre at best. Try doing a medical device without code reviews / PRs. Of course this approach can be improved, but after reading the full trunk baded development psge it was clear to me thst it is not for sny of my teams.
Love tour videos by the way!