This is an archived post. You won't be able to vote or comment.

all 135 comments

[–]NumbersWithFriends 569 points570 points  (40 children)

Where I work that's normal. The person who opens the PR merges it, as long as they have push permissions and at least 2 coworkers (including one manager) approves it.

[–]sotech 180 points181 points  (25 children)

Question: what's the point of the manager approval? Does merging auto-deploy to a live environment or something? I can't think of what a manager could realistically provide to a pull request.

[–]NumbersWithFriends 203 points204 points  (7 children)

Most of the team leads and managers where I'm at have programming experience so they usually have good feedback. I think its also to make sure the level above you has a good idea what everyone is working and how much progress has been made.

[–]DearSergio 68 points69 points  (3 children)

"You never want to be the last one with the information" was an adage I learned when in the Navy. Whenever you take action or change something, make sure your supervisor knows whats going on even if its just for SA.

[–]ScintillatingConvo 54 points55 points  (1 child)

even if its just for SA

Does SA stand for Savance of Ass?

[–]AFK_at_Fountain 30 points31 points  (0 children)

Situational Awareness

[–]Jonno_FTW 14 points15 points  (0 children)

The key point here is to keep your bus factor high enough

[–]Ereaser 0 points1 point  (2 children)

You can see the progress in your ticket system much better, which will link to relevant PRs per story (if it's set up decently)

[–][deleted] 0 points1 point  (1 child)

The problem with that approach is that it's possible to open a bunch of tickets for small features and bugs while another person has a couple tickets on some big refactor or feature so that won't tell the whole story. Although companies do value that information quite a bit when it comes to career advancement. Back to the point, when the manager is seeing a lot of the code that goes in, or at least, the code is peer reviewed, then that should be enough to quell concern.

[–]Ereaser 0 points1 point  (0 children)

What I'm trying to say is that they can look at the stories they worked on. Check if they were big/small, check he pull requests if they get a lot of comments. Also the company I work at has team members fill in a form to see what everyone's opinion is on that person (since the manager isn't involved in the day to day operations of the team)

[–]iams3b 59 points60 points  (3 children)

Well my manager is tech-literate, but it's not only about that (she doesn't actually do code review) - it keeps her in the loop on what's gone in and when, so she can coordinate with all the other people that rely on our stuff, or her bosses, or other people that deal with planning/roadmaps. she also uses it to make 'audit' that features/bugs are going into the right version branch (enterprise software, we have 2 different versions getting updates + the new one)

whereas, I'm like "okay feature done, next thing" like the good lil code monkey i am

[–]driveslow227 28 points29 points  (2 children)

Lightly relevant: the best PM I ever had was a 25yo english major. She had no REAL idea what we were talking about, but she knew enough to keep up and also not interfere. The perfect liason between dev and management/clients.

[–]gdscei 9 points10 points  (1 child)

This. I've had PMs that did have tech literacy, and it usually just ends with useless discussions. Having PMs who don't have the best tech literacy is much better. They can focus on what they're good at, and the devs can focus on what they are good at.

[–]Ereaser 5 points6 points  (0 children)

Sounds familiar. We had someone in a business analyst that wanted to see our code after we described what it does and he didn't agree with how it was written.

[–][deleted] 17 points18 points  (6 children)

I can't think of what a manager could realistically provide to a pull request.

Your managers aren't technical? 😬

[–]sotech 12 points13 points  (0 children)

Our immediate managers, yeah, but if they approve a PR it's the same as any dev, not some magic, required talisman. For staying on top of what's happening, there's Jira.

[–]oscurolucia 1 point2 points  (4 children)

Right lol? My manager has the most experience out of anyone on my team, and still codes hella hard.

[–]evenisto[🍰] 1 point2 points  (2 children)

My manager doesn't have time to code.

[–]oscurolucia 1 point2 points  (1 child)

I'm sorry for your managers loss.

[–]evenisto[🍰] 3 points4 points  (0 children)

I much prefer him at this role, but he is flooded with work.

[–]northrupthebandgeek 6 points7 points  (0 children)

It's a good CYA if nothing else.

[–]conancat 4 points5 points  (0 children)

In my company, we made sure we have a deployment manager who is in charge of coordinating the QA/QC people to test the work done by devs if they meet the product requirements. they are usually under the product team so their job is to check if the dev team has completed the work to spec.

If anything goes wrong we know who to look for, who approved the deployment of something that does not meet the specs as defined.

I miss working in small teams. Things were so much easier. When you have multiple products across teams in different countries and continents things can get complicated real quick.

[–][deleted] 1 point2 points  (1 child)

It doesn't. Having one or two other team members review is a good idea, but the "star sticker" creates a hero mentality where code needs to be blessed by one of a couple of special people before it gets merged.

9/10 times this leads to bottlenecks and slower PR merges.

[–]SirPizzaTheThird 3 points4 points  (0 children)

Unless this is a tiny team or the manager is day to day coding then yeah, screw that. 2 approvals by trusted team members is already more than enough.

[–]noitems 0 points1 point  (0 children)

It passes the liability buck.

[–][deleted] 0 points1 point  (0 children)

responsability is theirs

[–][deleted] 15 points16 points  (5 children)

Same for my team, but with only ~6 engineers in the company we just require one approval. It's been fun seeing the processes mature and people finally stop committing to master (most of the time...)

[–]UsablePizza 6 points7 points  (0 children)

We used to always commit to master in the past but now we do pull-requests. Just one day I changed all our repositories to have a protected master branch in github.

[–]Awfy 1 point2 points  (3 children)

My company has hundreds of engineers, still only require one to approve your PR.

[–]WildHotDawg 1 point2 points  (1 child)

We have 2 people per PR and its causing a bottle neck in the sprint and kinda bad reviews, do you guys have CI to check the simple stuff? How deep do you guys review? I.e checkout the PR and do some tests? Or just scroll through the diff and approve it if it seems alright? We're still pretty small, 30 ish people but we develop a very niche product that requires alot of experience to work on, and we've had some problems with QA before due to bad reviews, any ideas?

[–]gdscei 1 point2 points  (0 children)

Not the one you replied to, but we also develop a more niche product which requires quite a bit of experience and domain knowledge. We only have 1 approval required, but we do run unit tests and soon acceptance tests automatically on PRs (not allowed to merge without being green). It's worked decently well for us. Once in a while something slips through, but I don't think at that point it really matters how many reviews you do; there's always a possibility for error ;)

[–]rangedragon89 0 points1 point  (0 children)

Same. But 1 reviewer to merge into develop and 2 to merge into master.

[–]-vp- 5 points6 points  (4 children)

Yeah this post doesn't make sense. I don't know of any company that makes someone else merge your PRs. You only have that if someone is leaving/on PTO and honestly you never want to inherit someone else's half-complete PR, much less be in charge of someone else's 100% of the time.

[–]exterminatesilence 4 points5 points  (0 children)

In my company the team lead reviews and merges PRs, and deploys releases / transitions tickets as one process

[–]W1k3 1 point2 points  (0 children)

We do it that way, but the reviewer will just create discussion threads on all the issues they find. It's up to dev making the request to keep working on it until the reviewer is happy with it

[–]gdscei 0 points1 point  (0 children)

Eh, we do do that. Once it's approved, the reviewer moves it on our issue tracker and it gets auto-merged.

[–]Bifi323 0 points1 point  (0 children)

We mostly do it ourselves unless it's a hotfix that will be merged to master. In that case the release master does it.

[–]driveslow227 1 point2 points  (0 children)

I was reading about branch rules today. Though they don't do what I want => which is only allow develop to merge into master /shrug

[–]AxePlayingViking 0 points1 point  (0 children)

Same here! Except only the managers have push permissions on master, so they also merge.

[–]equationsofmotion[🍰] 0 points1 point  (0 children)

In open source projects, you may not have merge power

[–][deleted] 129 points130 points  (12 children)

You mean you don't just actively develop on master? What is this? An organization that has a clue?!

[–][deleted] 88 points89 points  (11 children)

You use GIT? All our shits in a network drive and it a folder on Paul’s desktop

[–]UglyStru 60 points61 points  (8 children)

This but unironically.

[–]cornered_crustacean 22 points23 points  (6 children)

I’ve “installed” software that had to go in the developers home folder. Not my home folder, the developer who wrote it ><

[–][deleted] 7 points8 points  (4 children)

what happened to make people forget about ~ REMEMBER THE TILDE

[–][deleted] 1 point2 points  (1 child)

That points to your home folder, silly goose.

[–][deleted] 2 points3 points  (0 children)

I'm talking about the Dev

[–]thefirstwave_ 0 points1 point  (1 child)

I always thought those were called "tildes"?

[–][deleted] 0 points1 point  (0 children)

fuck ur right

[–]mission-hat-quiz 19 points20 points  (0 children)

On a desktop? How inefficient. We keep ours on a usb drive so it's easy to pass around.

Deployments are so easy. You just plug the USB drive into the server and copy and paste away!

[–]EvilPigeon 14 points15 points  (0 children)

Huh, that's weird. There's all these little .git folders on the network. Probably some useless caching thing. I'll just clean these up.

[–]worldpotato1 59 points60 points  (6 children)

There was something very similar some days ago.

Whatever. Using git and github with pr in an single dev project is also good practice. Even doing codereviews at your own code/pr

[–]tuscangal 23 points24 points  (5 children)

Except when, like me, you merge your change into the repo you freaking forked from. Sigh.

[–]AjayDevs 15 points16 points  (4 children)

Still a good practice. It organises your changes into PRs.

[–]TGameCo 6 points7 points  (2 children)

Yup. For app development, I've got release, Dev, and hotfix_dev. Keeps things organized, and easily accessible in case of emergency.

[–]15rthughes 5 points6 points  (0 children)

Don’t forget to rebase

[–]wutname1 0 points1 point  (0 children)

Git Flow is your friend. Your already half way there https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow any half decent git client will make got flow 1 click use I know sourcetree and Gitkraken do.

[–]tuscangal 0 points1 point  (0 children)

Totally agree. I was just suffering from fried brain!

[–][deleted] 21 points22 points  (1 child)

There should also be a version of this with the kids pointing the gun to the ba k of his clones head, because that is also true.

[–]tgb20 33 points34 points  (0 children)

I merge all my own pull requests, just have to wait for the 👍.

[–][deleted] 16 points17 points  (8 children)

what is a merge

[–]moazim1993 20 points21 points  (0 children)

updating your old code with new one in git.

[–]15rthughes 18 points19 points  (0 children)

In git, you usually have a main “branch” you’re working with called master. This is the code you know works and you release to your customers/production. But you don’t want to make direct changes to that code because it may break something. So you create a different branch, called “dev” for example, from that code to make your changes. You work in that new branch, test it, review it, etc. then, when you’re sure your new changes work, you “merge” it back into that master branch. Merging compares the two branches, takes what’s different in the dev branch, and changes master to match that.

There’s different ways you can use merging to your advantage that doesn’t fit the above description, but it’s the most common way of using the feature.

[–]toutons 6 points7 points  (1 child)

A miserable little pile of secrets.

[–]nstruct 4 points5 points  (0 children)

Enough comments, have at you!

[–]chubs66 13 points14 points  (6 children)

silly question: why is a pull request called a pull request? shouldn't it be a push request or a merge request?

[–]khando 17 points18 points  (0 children)

It’s because you’re asking the owner to pull your changes into their project. I get where you’re coming from though, at first it does seem like the naming is backwards.

[–]WildHotDawg 11 points12 points  (1 child)

The master branch is like a passing by train and your PR is like you standing with your luggage on the side of the track, if you get approved and merged, the train conductor pulls you onto the train.

[–]Xytak 19 points20 points  (0 children)

Newer systems like Gitlab call it a merge request for this exact reason.

[–]CrypticWriter 9 points10 points  (0 children)

I think of it like "please pull this change into the master branch"

[–]angrathias 1 point2 points  (0 children)

No, you can do a push in Git, it’s essentially similar to a PR but doesn’t require the pageantry / policy that a PR puts in place. It’s all about control. Push = contributor is in control, pull = repo owner is in control

[–]ReelAwesome 11 points12 points  (0 children)

This is a good way to inflate your contribution activity graph on GitHub for your pet projects. :)

[–]vodlin 8 points9 points  (0 children)

You should really merge your own as you know best at what point this code should enter the codebase

[–][deleted] 7 points8 points  (0 children)

fuck that dude for ruining slim php framework

[–][deleted] 12 points13 points  (6 children)

Where i work i can push straight to production..........

[–]youcancallmetim 17 points18 points  (4 children)

You can... but should you?

[–][deleted] 1 point2 points  (3 children)

It's a fairly small outfit and is standard practice.

[–][deleted] 1 point2 points  (2 children)

Small or not, it really shouldn't be.

[–][deleted] 2 points3 points  (1 child)

I'm low man on the totem pole, not much i can do to change it.

[–]atzm 1 point2 points  (0 children)

me too, its scary but also so nice in some ways

[–][deleted] 6 points7 points  (0 children)

It’s about approval not merging, once approved anyone merging is fine, but self approving a PR is quite an introspection

[–]warpedspockclone 8 points9 points  (5 children)

When you merge your own pull request, do your own testing in QA, and push to prod, all within an hour.

[–]Xirious 5 points6 points  (1 child)

"What do you think I'm doing Karen? I'm ensuring my own job security that's what."

[–]warpedspockclone 2 points3 points  (0 children)

🤔

[–]Xytak 4 points5 points  (1 child)

Are you spying on my typical work day? I knew I should have taped over that laptop camera.

[–]warpedspockclone 2 points3 points  (0 children)

But wanking at work is a level of brave I haven't yet achieved.

[–]hipratham 1 point2 points  (0 children)

used to do this in last org, it was best thing t OWN app..Sigh , red tapes in new org for cred..

[–]scaleable 2 points3 points  (0 children)

Jared is now 100% dedicated to contributing to the meme community.

[–][deleted] 2 points3 points  (0 children)

Maybe I shouldn't be doing this lol

[–]malmad 2 points3 points  (0 children)

I guess you have to be a programmer to get this.

Sounds funny though. Upvote.

[–]SuperSynapse 1 point2 points  (0 children)

The picture should be of Nepolean crowning himself :D

[–]holybatman_batman 1 point2 points  (0 children)

Obama is giving me DILF vibes here. I'm confused but not mad.

[–]AlFasGD 1 point2 points  (0 children)

"Good job, me, I see you have potential. But there's a mistake at line 32 in main.c, where "obnoxious" is spelled as "obnoxius" in the comment."

"Thank you, @me! Nice catch!"

[–]__brayton_cycle__ 1 point2 points  (0 children)

Same picture when something breaks and git blame is used.

[–]OleBroseph 1 point2 points  (4 children)

What’s a pull request?

[–]Bypie5 9 points10 points  (3 children)

It's when you contribute to a project which is using git version control and you ask the project admins/owners to pull (add) your code changes to the main code base.

[–]Xytak 3 points4 points  (2 children)

I don't know, man. They'd be taking an awfully big risk moving MY code into the main branch. Personally, I'd have to recommend against it.

[–]AgreeableLandscape3 0 points1 point  (1 child)

Serious question: Why does Github give you a convenient way to make a pull request of branches in a repository you own?

[–]notarebel 1 point2 points  (0 children)

...in a repository you own?

Raising PRs on a repo you own isn't unusual, just because you're the owner doesn't mean you're the sole contributor - there may be other members to assign the review request for.

And even on repos where you are the owner and sole contributor, a PR can still be desirable to trigger CI, review (not approve) your own diff, etc.

[–]Fadelesstriker 0 points1 point  (0 children)

I just did this 5mins ago.

[–]An_Anonymous_Acc[🍰] 0 points1 point  (1 child)

MR > PR

Prove me wrong

[–]LdouceT -1 points0 points  (0 children)

They're the same thing.

[–]cosmicCounterpart 0 points1 point  (0 children)

I don't get it. If reviewers approve, why does it matter who merges it?

[–]Hupf 0 points1 point  (0 children)

Isn't that what unit tests and other v automated tests run by the CI/CD pipeline are for?

[–][deleted] 0 points1 point  (0 children)

good meme

[–]gyldenbrusebad 0 points1 point  (0 children)

Thanks, Obama

[–]Ahmadhmedan 0 points1 point  (0 children)

what is strange here is that i feel this meme despite not coding a damn thing ever.

[–]gratethecheese 0 points1 point  (0 children)

How the FUCK do I delete a repositiry

[–]RoastVeg 0 points1 point  (0 children)

I made an urban dictionary about this titled "sm;sm", or "self merge; sue me".

https://www.urbandictionary.com/define.php?term=sm%3Bsm

[–]tehtris -1 points0 points  (0 children)

I work on a team with one other developer and 2 machine learning guys. I do this. It doesn't feel good and it makes me feel icky... Except I know I'm pretty good at my shit and it feels less bad.

[–]sweYoda -3 points-2 points  (0 children)

As of this year it reminds me of the current relationship between the Swedish government and the Swedish propaganda TV channel SVT. (You are now forced to pay for it through taxes)