6 ms·

Reverse Pull-Requests

8 points by max-m 4y ago

max-m 4y ago
I’m not entirely sure if I got this right, but this system sounds a bit … weird. Everything is “tested” in production and then, at some arbitrary time, another team does a code review of the code that’s already in production and possibly broken? It just seems completely backwards.
- wahnfrieden 4y ago
  Sounds like real innovation toward actual continuous integration and continuous deployment! Indisputably better for the typical DORA DevOps metrics, the trick being relying on other quality controls and not treating code review as a quality control for regressions outside of feature flags. We operated this way back in the day building Canv.as and DrawQuest but without the snapshotting, that's a nice development Wish more teams would explore this type of development. Everyone is stuck in the git kernel dev model practices. It probably sounds weird because it's unusual, not because it's a DevOps bad practice (it's quite on the leading edge).
  - deleted 4y ago
    [deleted]
  - x3n0ph3n3 4y ago
    It also won't pass a SOC 2 audit if people can just commit anything and ship it to production with no oversight.
    - wahnfrieden 4y ago
      https://news.ycombinator.com/item?id=32697298 https://news.ycombinator.com/item?id=32697298
- jdlshore 4y ago
  It sounds like they’re using XP, or something similar. In XP, the code is reviewed and tested as it’s written, using pairing or mobbing for the review and TDD for the testing. (XP also uses frequent pair switching and collective ownership to get more eyes on the code, but it’s not clear to me whether they are or not.) So when the code is deployed, it’s already been reviewed and tested. The review they’re talking about here comes later, as a way of keeping the team in sync. It’s not the main code review mechanism.
kcb 4y ago
Do people really want to work in pairs all the time? I feel like it would get tiring if not just an occasional thing. There's also value in being able to be an "individual contributor"
- mkl95 4y ago
  I'm OK with pairing a significant amount of time if it's async. I find traditional sync pairing draining after an hour or so. If you are pairing all the time something has to be off, it's not efficient at all.
  - merlincorey 4y ago
    How exactly is it still "pairing" if you are doing it asynchronously? My understanding and experience with "pairing" is that it pretty much has to be synchronous because generally at least two people should be looking at the same screen while generally one is providing inputs (mouse, keyboard) and the other is providing high level guidance and looking out for low level mistakes. Doing it asynchronously seemingly would not dispense with sharing the screen and one person handling inputs which seems like it would be even less efficient.
    - marcosdumay 4y ago
      I imagine they mean when you work with some high bandwidth channel with another developer, and work independently on the same thing.
    - dieselgate 4y ago
      One type of async pairing I’ve done is “one person writes tests and the other writes the code to pass said tests - then swap” Im sure there are others but this is all I’m familiar with - it works ok and has the benefit of being sorta TDD, or at the least making sure tests are written
- wahnfrieden 4y ago
  this branch strategy works pairing or not
itslennysfault 4y ago
This is the worst process I've probably ever seen. Only one person (or pair) can work on the code (in main) at a time for this to work. It's more effort than just making a branch and merging it to main, and it buys literally nothing. What is wrong with just creating feature branches and PRing them to main? It's literally the default workflow. Then when you merge to main you run the CI/CD just like this, but without having to go back and create a "snapshot branch" of main before you started. Also, as you scale you almost certainly have commits from multiple people in main by the time you do this breaking the whole "reverse PR" process.
- wahnfrieden 4y ago
  You're misreading it, you review in batch and in retrospect after it's been put into production (with associated incremental/canary rollout, feature flag etc safety controls) and trunk progresses freely
  - x3n0ph3n3 4y ago
    Good luck passing a SOC 2 audit with that kind of workflow!
    - wahnfrieden 4y ago
      What happened to curiosity for finding secure ways to push workflows forward? People said the same things before continuous deployment caught on and now that's common enough and definitely SOC 2 capable. I believe it's doable with production rollout controls that limit customer exposure to the code changes and the code's access to data (possible these days with things like pg row level security controls, and blue/green routing etc. - already valuable practices), and ensuring the batch code reviews happen before customer rollout. Even with these controls and incremental rollout, there is tremendous benefit to getting the code deployed into production environments and enabling this review workflow. The point then is to satisfy remaining security controls while in production, under these type of controls.
    - jdlshore 4y ago
Existenceblinks 4y ago
You could merge PRs branches (not PRs itself) into a shadow-master branch. Every push to any PR creates a fresh shadow-master with selected (labeled) PRs's branches merged. We call this an acceptance branch. Each acceptance branch could have its coresponding staging server. You could have several acceptance branches with different PRs merged. Each PR can be included/excluded (labeled/unlabeled) back and forth from the acceptance branch (get a fresh one on every label attach/detach) That's what companies I worked with do. And I even built a SaaS app for that, though they don't want to publicly advertise the app.
- kcb 4y ago
  Similarly gitlab supports merge results pipelines. https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html#enable-merged-results-pipelines https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipel...
  - Existenceblinks 4y ago
    Yes, partly! > Merged results pipelines can’t run when the target branch has changes that conflict with the changes in the source branch. Yeah, the app I built even use git's `rr-cache/` (rerere feature) and have gui for resolving conflict and marking it as resolutions. Pretty neat.
Brian_K_White 4y ago
This idea is ahead of it's time. By which of course I mean that I expected it to take a few more years before a new generation forgot where we all came from to arrive at git, and reinvents cvs for no other reason than to work the opposite of git, because by then git will be old and whatever is the opposite of something old is new and "progress".
throwawaylinux 4y ago
Why do people try to come up with these byzantine git workflows? > However, what was missing was wider visibility of changes being applied to the code. That is what is said to have been missing. The solution was not to use tags or feature branches, gitk or other visualization tools that could easily show changes being applied to code from any point to any other point. It was to make up this "reverse pull request" Rube Goldberg scheme.
spprashant 4y ago
Just reading this article made me nervous. More microservices than programming pairs. Deploying straight to production. Maybe I just like the old school simplicity, but this seems like a terribly convoluted process, to justify some sense of development momentum that people seem to be addicted to.
bobek 4y ago
This looks like a try in recreating audit workflow (in a bit overcomplicated way). https://secure.phabricator.com/book/phabricator/article/reviews_vs_audit/ https://secure.phabricator.com/book/phabricator/article/revi... Yeah, I do miss Phabricator :(
alkonaut 4y ago
This has one glaring problem (only works for one feature). The article mentions it, says “wasn’t a big problem for us” and that’s it. The real solution might be to create an after the fact feature branch and cherry pick all the relevant changes. That has some risk of breaking under high churn, but the feature branch doesn’t even need to build or pass tests, only convey a design, I suppose. But in the end: why not just use feature branches? As soon as the number of developers become big enough to worry about passing knowledge around, waiting 12 or 24h for changes to land in master is a tiny nuisance compared to broken master builds or reverse PRs.

8 points by max-m 4y ago

I’m not entirely sure if I got this right, but this system sounds a bit … weird. Everything is “tested” in production and then, at some arbitrary time, another team does a code review of the code that’s already in production and possibly broken? It just seems completely backwards.

wahnfrieden 4y ago
Sounds like real innovation toward actual continuous integration and continuous deployment! Indisputably better for the typical DORA DevOps metrics, the trick being relying on other quality controls and not treating code review as a quality control for regressions outside of feature flags. We operated this way back in the day building Canv.as and DrawQuest but without the snapshotting, that's a nice development Wish more teams would explore this type of development. Everyone is stuck in the git kernel dev model practices. It probably sounds weird because it's unusual, not because it's a DevOps bad practice (it's quite on the leading edge).
- deleted 4y ago
  [deleted]
- x3n0ph3n3 4y ago
  It also won't pass a SOC 2 audit if people can just commit anything and ship it to production with no oversight.
  - wahnfrieden 4y ago
    https://news.ycombinator.com/item?id=32697298 https://news.ycombinator.com/item?id=32697298
jdlshore 4y ago
It sounds like they’re using XP, or something similar. In XP, the code is reviewed and tested as it’s written, using pairing or mobbing for the review and TDD for the testing. (XP also uses frequent pair switching and collective ownership to get more eyes on the code, but it’s not clear to me whether they are or not.) So when the code is deployed, it’s already been reviewed and tested. The review they’re talking about here comes later, as a way of keeping the team in sync. It’s not the main code review mechanism.

kcb 4y ago

Do people really want to work in pairs all the time? I feel like it would get tiring if not just an occasional thing. There's also value in being able to be an "individual contributor"

mkl95 4y ago
I'm OK with pairing a significant amount of time if it's async. I find traditional sync pairing draining after an hour or so. If you are pairing all the time something has to be off, it's not efficient at all.
- merlincorey 4y ago
  How exactly is it still "pairing" if you are doing it asynchronously? My understanding and experience with "pairing" is that it pretty much has to be synchronous because generally at least two people should be looking at the same screen while generally one is providing inputs (mouse, keyboard) and the other is providing high level guidance and looking out for low level mistakes. Doing it asynchronously seemingly would not dispense with sharing the screen and one person handling inputs which seems like it would be even less efficient.
  - marcosdumay 4y ago
    I imagine they mean when you work with some high bandwidth channel with another developer, and work independently on the same thing.
  - dieselgate 4y ago
    One type of async pairing I’ve done is “one person writes tests and the other writes the code to pass said tests - then swap” Im sure there are others but this is all I’m familiar with - it works ok and has the benefit of being sorta TDD, or at the least making sure tests are written
wahnfrieden 4y ago
this branch strategy works pairing or not

itslennysfault 4y ago

This is the worst process I've probably ever seen. Only one person (or pair) can work on the code (in main) at a time for this to work. It's more effort than just making a branch and merging it to main, and it buys literally nothing. What is wrong with just creating feature branches and PRing them to main? It's literally the default workflow. Then when you merge to main you run the CI/CD just like this, but without having to go back and create a "snapshot branch" of main before you started. Also, as you scale you almost certainly have commits from multiple people in main by the time you do this breaking the whole "reverse PR" process.

wahnfrieden 4y ago
You're misreading it, you review in batch and in retrospect after it's been put into production (with associated incremental/canary rollout, feature flag etc safety controls) and trunk progresses freely
- x3n0ph3n3 4y ago
  Good luck passing a SOC 2 audit with that kind of workflow!
  - wahnfrieden 4y ago
    What happened to curiosity for finding secure ways to push workflows forward? People said the same things before continuous deployment caught on and now that's common enough and definitely SOC 2 capable. I believe it's doable with production rollout controls that limit customer exposure to the code changes and the code's access to data (possible these days with things like pg row level security controls, and blue/green routing etc. - already valuable practices), and ensuring the batch code reviews happen before customer rollout. Even with these controls and incremental rollout, there is tremendous benefit to getting the code deployed into production environments and enabling this review workflow. The point then is to satisfy remaining security controls while in production, under these type of controls.
  - jdlshore 4y ago

Existenceblinks 4y ago

You could merge PRs branches (not PRs itself) into a shadow-master branch. Every push to any PR creates a fresh shadow-master with selected (labeled) PRs's branches merged. We call this an acceptance branch. Each acceptance branch could have its coresponding staging server. You could have several acceptance branches with different PRs merged. Each PR can be included/excluded (labeled/unlabeled) back and forth from the acceptance branch (get a fresh one on every label attach/detach) That's what companies I worked with do. And I even built a SaaS app for that, though they don't want to publicly advertise the app.

kcb 4y ago
Similarly gitlab supports merge results pipelines. https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipelines.html#enable-merged-results-pipelines https://docs.gitlab.com/ee/ci/pipelines/merged_results_pipel...
- Existenceblinks 4y ago
  Yes, partly! > Merged results pipelines can’t run when the target branch has changes that conflict with the changes in the source branch. Yeah, the app I built even use git's `rr-cache/` (rerere feature) and have gui for resolving conflict and marking it as resolutions. Pretty neat.

Brian_K_White 4y ago

This idea is ahead of it's time. By which of course I mean that I expected it to take a few more years before a new generation forgot where we all came from to arrive at git, and reinvents cvs for no other reason than to work the opposite of git, because by then git will be old and whatever is the opposite of something old is new and "progress".

throwawaylinux 4y ago

Why do people try to come up with these byzantine git workflows? > However, what was missing was wider visibility of changes being applied to the code. That is what is said to have been missing. The solution was not to use tags or feature branches, gitk or other visualization tools that could easily show changes being applied to code from any point to any other point. It was to make up this "reverse pull request" Rube Goldberg scheme.

spprashant 4y ago

Just reading this article made me nervous. More microservices than programming pairs. Deploying straight to production. Maybe I just like the old school simplicity, but this seems like a terribly convoluted process, to justify some sense of development momentum that people seem to be addicted to.

bobek 4y ago

This looks like a try in recreating audit workflow (in a bit overcomplicated way). https://secure.phabricator.com/book/phabricator/article/reviews_vs_audit/ https://secure.phabricator.com/book/phabricator/article/revi... Yeah, I do miss Phabricator :(

alkonaut 4y ago

This has one glaring problem (only works for one feature). The article mentions it, says “wasn’t a big problem for us” and that’s it. The real solution might be to create an after the fact feature branch and cherry pick all the relevant changes. That has some risk of breaking under high churn, but the feature branch doesn’t even need to build or pass tests, only convey a design, I suppose. But in the end: why not just use feature branches? As soon as the number of developers become big enough to worry about passing knowledge around, waiting 12 or 24h for changes to land in master is a tiny nuisance compared to broken master builds or reverse PRs.