GitLab Code Review Weekly Workshop, 21 Oct 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Oct 21 2022

Description

In this session we discuss some interesting code review topics and pair on a database code review.

0:00 - Intro
0:35 - Reviewing when you cannot recreate the issue
5:30 - Catching issues caused by historical data
10:45 - Discussion about Exploratory Testing
17:50 - Pairing up on a DB Review
36:30 - Q: Do we review MR's with broken pipelines?
39:20 - Q: Pinging multiple people on MR's?
45:37 - Q: Ping issue authors on MR's for reviews?

A

B

Oh it's evening, I, don't even know anyways um thanks for jumping on the code review, weekly Workshop uh I have the first point on the agenda. So let me share my screen and my humble point is pointing out sometimes these Mrs that fix very specific, um very specific glitches in very specific environments.

B

So this is a crazy UI glitch only showing up in safari, but I can't recreate it in my Safari. So it must be only in some people's Safaris, uh but Simon could create it, recreate it uh and it looks like um I, don't think one of our team members I think this is just a um contributor contributed this issue that they were run into. Thankfully, someone on our team was able to recreate this on in their environment, um but we had a couple of iterations with the solution here.

B

uh So one when I'm reviewing this I could just Target hey. Does our change fix the issue and I can spend you know eight hours trying to really understand why this works on some environments and on other environments and that's not going to be a profitable use of my time, but instead of that uh um I decided to just you know, Simon's saying this fixes it. So that's great, um we'll see we'll after we merge it. We can see the original issue author.

B

If that fixed their issue, but I'm mostly concerned about, are we causing more side effects, unintended side effects with our change and the first draft we definitely did um so. Thankfully, I was able to catch that um by actually just running a quick smoke test. It was like. Let me just open up one of these components that we're touching and um thankfully it it showed up pretty quickly uh and then with uh the next. The next fix that happened to fix Safari's bug um was very non.

B

It was very no op e, so I was feeling pretty confident about it, but I went ahead, did smoke test and everything looked good, but I did leave like just a non-blocking question um just about the specific issue, because I can't test that, but I was able to confirm. There's, there's not any um issues here, so I felt confident merging it. Even though I don't really know if it's gonna fix the problem, I'm contrasting Simon but I think both of us are just trusting after we merge all this.

B

If the original issue contributor can confirm that this fixed it or not for them um sometimes sometimes that's the only way to really find out so I thought. That was uh interesting. Example is very um it's a very clear example of making profitability decisions when reviewing Mrs. So I just want to share that.

C

B

Yeah so I I I'm I have to protect myself from getting nerds sniped because.

D

Like sometimes I relate so much when.

B

Things don't behave like they should, and especially when it's like non-deterministic, there's something something inside my brain that wants to find the determinism and the non-determinant. It has to be that.

D

They're, an essays.

B

A

B

So funny um cool well, did anyone else? Have any other code review uh experience they'd like to share or code review questions I.

D

Feel like showing off someone else's good work. Oh.

A

That's beautiful idiom who.

D

Probably is asleep right now, so wouldn't actually be able to attend. This I had this idea just now, so give me one second, yes,.

A

D

Found the thing.

D

Oops there's a link here.

B

What are you eating? Chad.

C

I'm in a bed and breakfast in the middle of the Year Catan, uh it's delicious stuff, delicious.

B

Stuff: okay, that's my favorite thing to order at a restaurant.

B

uh It looks beautiful I'll.

D

Refine my comment in the document later: um oh cool thanks I will, let's share the screen now.

D

So absolutely no preparation here! Apologies for that um I'll click. My own link.

D

All right, so uh we had this change here, it's a security change, and so we made a choice that there was this. uh The the difference between tags and releases sometimes was not always clear, and we decided that, even if you don't have access to the source code repository, you can have access to releases unless you actually check the really like in the project settings you can say, releases are for project Members, Only and in this Mr there was this change.

D

uh Let's go into this View.

D

Where we just I'm gonna, let's have more more code. So uh when the repository is disabled, we use the disable releases because they kind of were linked together with this new way of thinking.

D

um This is not true anymore, so the EMR simply remove that line, which made a ton of sense, but shenya noticed that um projects where the currently, because the data in the database, um because we're making this change now there could be projects in the past. That would now have their stuff revealed and they never asked for it, so so so that was a that was a a good catch.

D

Where making a change now, it makes sense like if you create a project today with this code, everything is fine, but just having this thought of, how do our historical data behave now? This change happens.

B

That's a great that's a really good point well and.

D

Yeah, and sometimes this this in this specific case was this- would have caused a sort of minor but still security issue, but this causes real bugs, like just functional bugs as well. Sometimes you just break projects because you're like oh. No, this data actually doesn't work with our new models right.

A

D

So yeah that was a that was a good cache and just really thinking. Okay, like this change, makes sense, makes sense. Now, I tested it spun up my GDK creative project, click click, yeah, everything's, fine, that doesn't work on a five years old project and uh yeah, but it was a great catch by shinya foreign.

B

It's so easy to treat like new features or changes, just on top of like what we view, our GDK as being really thin and having no projects and we're just looking at new projects. uh But thinking about how the changes affect long-lasting projects is critical. Yeah, that's cool thanks for highlighting this.

D

So before we merge this, a migration was created to make sure that data was populated for all historical projects.

B

um Interesting, so let me ask you a quick question, then, so we're there's release access level.

D

But we're yeah I'm, going to uh let's I'll have to reread it myself. I have like this thought: I need to process it again. Well,.

B

No, my biggest question is like: if we realize okay, we've split up permissions, but then now we're gonna, combine them but I'm. Clearly not understanding of everything that's going on here, but what what instance do we need to notify users in current project owners?

B

Things they thought were private are now going to be maybe more.

D

Probably uh yeah that's a good question uh well since, because of this migration everything's going to to kind of stay as it was or.

A

D

Switching the people who had this disabled, we kind of Switched the other switch so that the behavior stays the same got.

B

D

uh So there's nobody is going to notice the difference, though. Now, if you mention it like someone who would be used to that, behavior would create a new project, and now things might behave differently. I think here the change was minor enough that yeah yeah it doesn't require a a widespread notification. Otherwise that would be that would be I guess like a a breaking change notification on a major version.

B

Yeah, that makes sense um now what this is interesting. This is a really interesting example and.

C

I give a a contribution along those lines. Please do I'm going to uh drop in the dock, a link.

C

C

Exploratory testing book explore it and uh exploratory testing is a great practice. This is a book by my friend, Elizabeth Henderson, who is a a master at this like if you watch her exploratory test, something things like this she'll find just the the question. She asks the and there's a methodical wage who go about it in this book, but asking these sorts of questions like what does this do for previous users? How does it change the behavior and tons of other things, and uh this is a great practice?

C

In addition, you know that really complements code review like how can I poke at this and break it and watching her like do live. Exploratory testing, for example, like in a seminar is he does amazing, like she'll, take your code and she'll like what, if I poke it here and then oh I'm, well what if I poke it here and you're like you're, breaking my code in ways that I didn't even know there was code? How did you find that? Oh.

B

Man, wow, that's cool. um Does she uh is this also with um dive into like white box testing techniques, where you see the code yeah.

C

And um like at pivotal, you know where uh she helped us a lot. We had at the parent station, just handouts that were uh lists of things that you could go through when exploratory testing to try. You know well security, around usability and other things like that, it's sort of a heuristics and reminders to poke things. That might be. You know a good addition to review your checklist or something in our handbook. Maybe.

B

That's a good idea of um and I think uh I mean this exploratory testing bit is like the most important thing, I feel like we do as reviewers linting catches most most of the stuff. We hire really good Engineers that catches a good amount of other stuff.

C

Right and like this is an area that just I can't really cover be covered by automated testing, but it's extremely important right at top of the pyramid test. You don't want to have a ton of those because they're so expensive and Flaky and the bottom unit tests that the developers write Vape. You always have a blind spot, because it's your code, I'm not going to imagine the things the plugs that I haven't found because I don't have a good imagination. It's my code right.

E

Yeah I feel like the is this known channel is Almost should be renamed like exploratory testing Channel, because it's literally what people are doing and a lot of the time they catch things that can't be caught through automation.

E

um A lot of the stuff I honestly, wouldn't notice, even if I saw it as like the spacing of things just as a back-end developer and.

C

You have a testing background, so you're.

E

Really good I mean I love breaking stuff, that I I approach Mrs by like how can I try to break this, but just like I don't know if that's a good way or a bad way, but um I just feel like the way that yeah.

D

E

We really good at breaking stuff. So that's what I try to do when I'm reviewing things.

A

B

Yeah, it's a good way, there's a very classic joke of, like the QA engineer, goes to the bar and orders, zero beers and 99 000 beers, and all that uh the first user goes to the bar and asks where the bathroom is and it catches on flames and explodes um yeah. No exploratory I was trying to find a picture of it, but the testing pyramid there's diagrams of it with the with the All-Seeing Eye at the top of the pyramid that I don't what is it the? What is the Freemason? The.

C

B

Yeah, um meaning of just like that expert driven exploratory testing is so important at knowing what needs to be pushed down. The automation, testing pyramid uh and.

C

Yeah, that's a great visual, like the the omnipotent eye is like above the pyramid. It's outside the pyramid, yeah.

B

Yeah, but it's um I think I think different organizations treat this so differently. It's like you can get a QA tester. That's looking at the requirements, doesn't see the code at all and is trying to punch at it as they would expect a user would.

B

But, as software Engineers like, we tend to lean more towards being comfortable with the code and less of putting ourselves in like the users shoes, but I I want to check I'm, probably going to get the book that you mentioned Chad. It's like I love. That's my favorite way to review code. If I see a function and I'm like there might be issues here, it's like okay, I need to try to from the user's perspective reveal why this approach doesn't work or whatever yeah and.

C

Elizabeth has been doing this, for you know, decades, probably uh I'm sure she's got some good videos. You know conference presentations and stuff online too.

B

And I'd love to I'd love to get some more language and ideas about that and I. Think that's where this expert driven exploratory testing, QA and ux are really good at being empathetic with the user, but us Engineers should be really able to really understand the code and be experts at the code and and that that gives a whole new perspective on what to test and what things could go wrong.

B

um That's really cool thanks! Thanks for bringing this up Chad thanks for the recommendation.

A

B

Let's see anything want to have anything else, they would like to add to code review, show and tell them questions.

A

B

Well, this brings us to the um the part of the episode where we uh um sing a song. No, where we do code review pairing, um do you have any code review that they want to pair on? We can all pair on together.

B

I have one but it'd be a front end one uh and it's removing a feature flag. So it's kind of relevant to.

E

um I'm looking to see what I have queued up, at least I- don't have seven, so that was good.

B

I'm gonna be honest: Terry I feel, like I feel like you're. Holding back on me.

E

Oh, this one's all deletes of code. So I guess that's.

B

That's where I'm at too.

E

No yeah, that's when it's like I think it's only. It adds one line and deletes 158, so I don't think that one's pretty interesting. Oh.

B

It's all interesting.

E

um I don't know what else I have.

C

I mean that that is an interesting case, because how do you know about it? I.

E

Looked at it, so this is a.

C

Completely shut.

E

Down this morning- and this is in my queue- we can look at it if you want and then y'all can help me do my work. Let's.

A

Do it and that's pretty cool? Yes, that's the intent, yeah.

E

um Okay, let me see I am assigned as a database. Reviewer I would say: um I am not super and I'll get it up on my screen in a second I. I am not at the point where I even want to try to become a database maintainer. That's where I'm at with my database review skills right now, so we'll see how this goes.

B

Okay wow: this is great and.

E

Let me make it a little bit bigger because screen size okay, so um this is removing an old back fill projects with coverage, migration for the CI decomposition uh from what I can tell I guess this is related to when we moved the CI database tables to like their own database.

E

Any migrations of 415 that don't change the schema can be, you know, opt as we assume they must already. This.

C

Is really interesting like what what I used to do on Rails project is like we periodically collapse the migrations, like things that were more than a year old, we just collapsed them into. It was basically just a single initial schema, and then you didn't have to worry about this sort of stuff. You know like the data was there or not so.

E

We don't do that here. Yeah.

C

It might be a good idea, it would avoid. You know issues like this, that we need to exist so.

B

um I think we do a level of that. uh Let me see at our migrations folder like what our latest.

A

E

Know like we do that elasticsearch has migrations, that we've been writing for the search team and we do do that. Every Milestone we like mark them as obsolete at the dot relief like the dot Zero release, so that okay, once you get to this point, we can assume everything else has been run.

C

Anyway, that's not directly related to review. We don't have to talk about that. Well,.

E

B

Yeah, it's a good question, though it does look like um we do something similar. So it's like our earliest DB migrate is 2021 March, 1st I.

C

Think, okay, so we do do that. I.

B

C

That should be like a giant one with, like the whole schema in it right. As of that point in time,.

B

uh It is a really small file because it reads from init structure: SQL yeah. So it's got to be.

C

Really, okay, so yeah there you go. We do that. It's maybe not often enough to solve.

B

This well, when it says old, because that's a good question, because it's saying this is an old migration. It's not that old.

A

E

um This one, no, it's not uh 20 2108.

C

So, maybe that's an answer for this one like, rather than dealing with these things, just do a collapse. You know to the last supported release whatever, however old, that is.

A

E

Yeah I think so I'm not gonna. Look out. This is all just deleting code, I guess the question, and it's not so much I guess for me because kind of like is this code? Fine, because all they're doing is working it as a no op um and deleting the related specs I guess for me it would just be if I was going to review this I'm. Looking at what.

E

I guess what issue it's trying to um fix? Well, this is a year old, okay and there's two background migrations. It's like one of them's already been taken care of.

B

We will not update older background migrations to work properly with decomposed databases.

E

Yeah yeah I guess the reasoning is this saying? Basically every anything that's been run everything that, um let me see this migration was merged in 143, so this should already be run before upgrading to 15.x and then why it's being removed because of the active record-based connection, query a CI table and while it could be fixed, it's simpler tradition for a little bit, because.

C

We we forced them to upgrade to Major versions incrementally, so it doesn't matter yeah.

E

They have to go to like 14, whatever the last 14 before they get to 15-0, and so that makes sense.

B

This is like going from Windows 7 to Windows 8.

E

Just throw the computer out the window at.

B

That point, but there's no Windows 9 is there. Is there Windows 9? It's not. It went straight to Windows, 10 I, think I.

C

Don't think I ever did a major Windows upgrade you always just reinstall. Next time we install Windows like every six months anyway, at least in the old days, yeah.

E

A

D

A

E

Think for this I'm inclined to just approve this I'm, just as the the the reviewer role but for I think this is fine. So.

C

Are there no schema changes in it? There's.

E

No schema changes, it's just deleting the migration code here and then um this is just all the related specs that got deleted so I think yeah. This is probably fine.

E

um A lot of the times as a reviewer for the database, I will say like I'll. Do my best I, don't feel super comfortable to become a maintainer, so a lot of times I do try to follow what happens later. To see what the maintainer says. We have some very thorough database maintainers, who are very good at SQL, um and they often have like leave a lot of good like insightful comments. So.

B

Can I can I ask you two two challenging questions?

B

um Okay lending, do you have any? Would you leave any questions like do you have any questions for this similar.

E

E

Don't think so I mean I guess um because, like reading through, this Dylan's left a lot of good information about like what happens, if someone's writing an old version, so that question would be answered like what happens. If somebody upgrades the 15-0, could this cause them or 15? What a six could this cause them problems and if they followed the proper migration path they shouldn't, um they shouldn't have any issues.

A

Well, I think they I.

B

Think they yeah you're right they wouldn't because they would have to go from 14o to 15 out, and so that means this is included in 15o right.

E

E

Included this is included. This was merged in 14-3, so as long as they've gotten past that and I guess, if they were on 14 1 and they yeah. If they wanted to go to 15 x, they would have to get to 14 11 or 12 or 10 or whatever the last one is I forget, because we had one year where there was like 12 and one year there was 10., not releases so I confused about how many we have but either way I feel like because it was important like I I.

E

Think if this was in like the last I, don't know if there's a release that it could be, and that would cause it to not be run before they upgraded I. Don't think so. So.

B

If a if a customer was on 14-0.

E

B

Then upgraded do they have to upgrade to 15L before upgrading to 15 6, or could they go 14-0 straight to 15-6, I.

E

Think they have to go to the last version of 14 before they can go up. Oh.

A

E

B

Well- and maybe this is a question um of of like- maybe we could look it up, but maybe also to just save time of being asking a question just a clarifying question of if a customer is on 14-0 are they do? Are they able to migrate straight to 15-6, and would this cause an issue because of that.

E

D

I'm in our upgrade path page and seems pretty complicated. It's like 40.

F

Minutes 40.

D

56 1495 14, 10 15.

B

So they have to deal all of these and and steps is that yeah.

E

I, don't think you can go direct from a major version. It's major version. You have to go at least that's with our migrations, like we rely upon the fact that you have to get to like hear 1410 before you can move Beyond it wow.

D

I had a link in the agenda.

E

D

C

Like that's, really the only difference possible to do it. Otherwise, yeah.

E

C

All the edge cases that could happen, yeah.

E

And I think almost you should see I, don't know how they these numbers of where you have to go to, but at least um when you're going to like, um like an earlier version and a Target version yeah there. It shows you kind of like where you do have to get to this. Like last 14 version.

C

Like a Choose, Your, Own, Adventure book and.

E

I think part of it is because when we do all the like big breaking changes in 15, they don't like you. Try to everything has to still work like the release or two before that. I think the previous release still has to kind of like work right to get them there.

E

A

E

Guess that would be my only that would have been my only question but I since I felt like because I have experience with like developing for the upgrade paths. I felt.

F

I would feel.

A

Okay, just saying yes to this.

E

B

That makes sense, yeah I, don't know um the other. The other thing I was thinking might be helpful, is looking at that migration file. It's be kind of weird to have checked in migration. That has two no Ops like the history of it is confusing. So.

E

B

Leaving a comment to an issue or leaving a comment.

E

Oh yeah, that's a good idea like should we leave a comment here.

B

I think that would be helpful for people that, like aren't familiar with migrations like myself, I've I'm gonna, get into like I'm gonna, get really confused when I realize that migrations can change.

C

That's like the good history like you could just rebase Master. If you really need to that's okay,.

E

A

E

I'll just leave that and then we can see what the maintainer says as well, um but that's a good point. I, don't always think about I, sometimes do I guess sometimes I'll leave questions to be like. Should we leave this here for any future developers who come in and say what happened here.

B

I also think of just Community contributors, sometimes just look for things that they can just clean up and so finding comments that like are no off. They might look at that whole file be like oh, maybe I could just delete this and yeah that I don't know. That's.

E

A good point: um okay, I.

C

Like it yeah like it's also why why do we even need to keep the file? Why do we not delete the file like I said side? Note I've been my my partner's learning, Django and their migrations are are way cooler than rails actually like they say which one they depend on, rather than just like relying on the name of the file but point being. Why do we even need to keep this around? If it's a no op? Can we just delete it?

C

uh I I, don't know the answer. I don't know, maybe there's a reason, but maybe you could ask that too. Okay.

E

Yeah I feel like when the last time I used to have a problem with my last job in Java. If you like, deleted migration files, and there was like some schema table database I.

C

Don't know oh yeah you're right, the schema migration just keeps.

E

It really excited that would like be like nope there's something missing.

C

um Yeah schema migration says what you've already read: okay, one breaks if you refer to something not there, but it's best to leave it. Okay, but.

E

That is a good question and honestly I didn't know. The answer. I knew the answer for my last job because I broke stuff, I didn't know the answer here: didn't break it in production, at least. So that's always a good thing, but I will leave this note for them. I don't know if I have any other reviews, but what I yeah? What I'll probably do is approve it and then pass it on to the maintainer and see what they think.

E

um I don't know if I have any other Mrs I can pass me. The Mr um yeah.

A

E

This is an interesting one. Thank you for letting me finally share a database one, even though it wasn't like the most involved database. One.

B

Oh my gosh no, this was this was incredibly done. Pipeline failed. There.

E

I see good eyes from Casia the pipeline failed. That's another thing that maybe we'll should look at.

E

D

E

E

You'll miss a bear. Oh hmm this.

B

Migrations validation, migration.

E

Is it from this migration? How.

B

Old is the pipeline.

E

E

uh 15 hours ago,.

B

um I was running into all sorts of really weird migration failures when um the on the fail fast job. This.

E

Is it yeah? This is the fail fast job, uh maybe I'll retry it and see. If.

B

We've seen they're migration related, but they're unrelated to this migration.

E

Yeah it looked like it was multiple migrations, um but I'll, give it a whirl and see if that fixes. It.

B

Are there any um like model properties that we're removing in.

F

A

E

And I think that wasn't the CI, but that was really good. I didn't see, I feel like shouldn't. Have this have been red? uh Maybe not when the pipeline is red.

A

E

What's interesting.

A

Is this is also a.

C

No, it doesn't I thought that was true for, like the first almost two years, I was here it's just whether it's open or closed or it's.

E

So weird I feel like there's some view where it turns something: I don't know if there was some some thing up here. That would show me this is: has a bad state of some sort, because it's yeah from like this view. It's just you can't there's no visual like indication that there's a.

C

Brokenness the pipeline down there, the the first thing uh right above that, like that they can emerge results. Yes,.

E

Oh okay, wow shoot um I didn't want to break down that far I guess, but yeah I'll.

F

Make sure this.

E

Pipeline is passing and I think at least one other thing. I've noticed from the reviewer side is I have stopped uh using the rebase I, don't know if anyone else has I do not rebase for other people, anymore, I'll start a new Pipeline, and that is it because otherwise you can't merge.

A

E

I, don't know how that's gonna play itself out, but I have.

C

Ran a discussion about that yesterday and I mentioners when you maintainers meeting right yeah.

B

That's that's causing some general maintainer reviewing grief.

E

Supposedly, you could just start a new pipeline um and that's supposed to fix things. I don't know if that's the case all the time, and there are other reasons you want to rebase. Besides, that.

B

Yeah there's there's some um there's some pipeline failures that seem to require rebase I, don't know what the distinguishing factor is between those I.

C

Mean there's a lot of them? Yes, if you're um like, since we don't use merch trains, there's many scenarios that yeah.

A

C

Doesn't match what's on your your branch or you've got to flake in or something.

B

But we do merged result, pipelines.

A

B

Every time we start a new pipeline, we'll do a new merged result, but for most situations, if like a math, broken master can get fixed on my Mr by re-running the pipeline. uh But these days I found Master to speed so flaky in all sorts of ways. It's like I rerun it and different jobs have failed. I, don't know.

E

um Yeah cassia had a question in the chat. Do we still review Mrs with broken pipelines.

F

E

I thought it was a good question, so obviously, what I do I often do review Mrs with broken pipelines, but then I also look at the pipeline to see. Does it look related and whether or not it is related is like I either leave the comment that oh, the pipelines, don't look related, but can you double check or hey this pipeline failure looks related? Can you check so either way the person is going to check, but um I think it's still I usually still.

A

B

B

What we're gonna say: Chad.

C

It says in a handbook: I, don't know, I, don't know if my internet's coming out, but it says in the handbook it's okay to ask for reviews early, so the pipeline may be broken yeah.

E

I also sometimes ask for review, while the pipeline's still running the first time.

E

um Well, if it's a small enough change, I'm like okay, let me get the review process started and then I'll come back like the next day, be like oh oops, you know, I missed the test or something but most of the reviewers will do still review it. Yeah.

C

As part of the efficiency value, you should totally be fine with doing that. In my opinion- and it says we're in the handbook, but.

B

Piggybacking on the efficiency value, if a pipeline is broken, and it's clear that this would probably significantly change the approach, because it's like super broken or broken in an unrepairable way, or it has legitimate issues with it. It's probably when there's a broken pipeline that might be best to be the first thing you look at of.

B

Do I even need to contribute more time into this, because maybe this whole path has been invalidated.

A

B

99 of the time, it's just chores that need to be done.

C

And if it's depends on the type of review, if it's an initial review or an unrelated domain specific review, then I'd say: okay, it's fine to let that start, but not necessarily maintain a review. Yeah.

A

That's a good question: well, uh turtles Torres, I,.

B

Don't know uh thanks for hanging out and talking about code review, uh yeah I know we're a little early but I'm I'm fine with ending early. If someone has any other questions or comments, you want to bring up. Otherwise we can uh call it.

A

Oh yeah, what's up.

F

I, don't know if you heard me I.

B

Did it I'm? So sorry, I was just gonna.

B

That's not a good analogy.

F

My internet is not the best these days, so it cuts off. Sometimes it's weird um yeah I was wondering um when we do a code review. Is there like a hard limit like please do not ping more than two or three people on a Mi? Just because we tried to save people's time or whatever the case is um yeah just.

B

F

B

Oh, why would pinging multiple people save more time, so I want to get more context with the question.

F

F

So uh I was thinking to Ping a few people in my team to have a look before I actually request a review from outside of the team, because people in the team might have a little bit more context on what's going on and um but then I thought that maybe it is against the efficiency value, for example, like people spending time on reviewing my code, given that they probably have multiple Mrs to be reviewed, uh etc, etc. So I'm just wondering like what do people do on this call?

F

Like do you review code that you were not pinged about, or do you always wait for a ping on the actual Mr.

B

Ask a question: um the Terry, Chan or Dominic. Do you have anything to add to the.

C

I I would say, like you can always nobody's obligated to do a review if you're too busy for it, you can just say no so I would never feel bad about asking for review like if somebody else or if, like three people have already reviewed it, and it's fine and you're just asking like if you don't have a good reason but I, you know I'm sure. That's not the situation. You're talking about you're really wanting to get more insight into it, but I, don't think there's any problem with asking.

C

What do you think Terry.

E

So yeah I think I often will queue up a few reviewers at once, um but it depends uh if I think a review might change the approach. I might hold off on asking too many people, because then you don't want to have to go back and be like. Oh you've already reviewed this, but then, like everything, changed so um I hate to say it depends because I feel, like that's a very bad answer, but I do look at like what's happening.

E

Sometimes I do want to queue up like documentation, review and the back end review, and maybe a front-end review if I, because if I'm getting all three of those, it could take a.

C

While only different domains, I I always do them.

E

All at once, like I, hear them, but sometimes you know ux review sometimes I'll ask for that. First, just to make sure that's gotten okay um and then I kind of like fan out from there, because I yeah I'm not that good at doing the front end stuff. So I'm like okay, please like make sure this looks okay before I get the other reviews, but yeah I think it's perfectly fine to to ask one an old person. Nobody is going to be upset about it.

B

Yeah I have some some advice for on this topic, um because I think, if I interpret your question correctly, it was not necessarily about pinging the reviewers for different domains, but almost like I have backend Engineers on my team and the review bot back in engineer and the maintainer I could ping them all at once. I could ping multiple backend engineers at the same time to reveal it, and my bit of advice would be if I wanted an MR to never get looked at, I would ping instead of pinging a person.

B

I would ping two people.

C

Bystander effect, oh.

B

My gosh, it is so real, um so I would I, would officially assign a domain expert if I feel like this needs. A domain expert of you, yeah, and sometimes it's in my team I really need you to review this, and if they can't they'll, say I can't get to this go ahead and just go to the main reviewers like okay, that's fine, but I'll also will just ping people to FYI we're making this change here and I think you may want to be aware of this, but I'm not expecting you to look at this.

B

So pings pings also have like a different weight to them of sometimes I'm, just see, seeing as FYI I'm changing this code that you kind of just touched and here's why it shouldn't affect you. But you need to be aware of this, that that happens a lot, um but for your own, um for your own efficiency, I recommend not pinging multiple back in engineers and asking anybody to pick it up because of experience that doesn't that doesn't get picked up. Yeah.

E

It happens in slack too, like when you're trying, I've I've had that happen to me where I'm like. Can anyone help me with this? You don't get any answers, so I'm gonna have to go, find someone directly with whatever experience and ask for direct help.

E

um Yeah I would say also like if you get in a back-end review from your team, that's the dri that counts as the initial backend review to me and then, as a maintainer I would say it. Fine everything looks good, so you wouldn't need to get the reviewer from the back end. If you already have someone from your team.

C

I think one more point I'd make is like it depends on the nature of the Mr. For example, one I have right now it was it's like a brand new CI job, that's sort of a weird case not like any other pre-existing one and I broke master on the first attempt at it like with Dr, William, Mars and fox and so I'm taking a second shot at it, and the rules are really hard to understand and there's no way to easily test them. So I'm definitely like gonna.

C

Try to ask multiple people to look at this, like I, really, don't know, I, think it's right this time, but I don't know, and it's such a niche of domain context. It is actually I'm having a hard time finding people to look at it.

B

Yeah, that's a um that's a good example. You ask a question in the chat to Casio. Do you want to verbalize it.

F

uh Sure uh so I was also wondering whether pinging or requesting, a review from the creator of the issue, whether that is recommended or not because in my personal opinion like if someone created the issue, they have, they have the biggest knowledge or context on that particular change. But what if uh the Creator is actually like a maintainer and not a back-end engineer so like?

F

If, if you know what I mean like I, yeah I,.

E

F

Get deflect like I, get deferred to someone else saying: oh I'm, not the first level back end. So.

A

F

B

Let me let me start by um by.

B

We should feel free to Ping anybody at any time and if anybody ever says don't ping me yet this hasn't been reviewed by someone someone of a lower caliber than me. That's a that's a culture issue and everyone should be receptive to getting pinged at any time. So, um but when you do ping someone, it's really helpful.

B

If I got ping to review something I'm gonna automatically think it's for a front end. Maintainer review, um so it'd be really helpful. If you want me to Ping something to review it as an issue offer adding that context like hey, could you check this out from this perspective? That's so help, um but at the same time I create lots of issues and I'm, not the I'm, not the expert at the issue. So because of our efficiency value, everyone can contribute and so lots of people end up.

B

You know: hey I found something weird, here's an issue team go, take care of it, and so sometimes the author may not necessarily be the best person to validate that. But.

C

In terms of exploratory testing, they at least know what the bug or the future is supposed to look like. So from that perspective there may be one of the best people to review it.

D

Yeah I, don't remember last time, I opened an issue and I was actually an expert at I.

C

Just do that all the time I think, regardless of who opened it most, people are like Overjoyed to see that somebody finally fixed an issue they opened, regardless of whether they know about it. So I don't think anybody would ever be mad.

B

Yeah I do really like that idea. Like I realized, I haven't I, don't I, don't really respond on issues a lot either like I need to, especially if we get issues open from the community. I think that would just mean a whole lot to them if we included them in the Mr and just let them be a part of the whole collaborative process that that's a really cool idea. I love that idea um so I appreciate bringing it up. I hadn't really thought of that.

C

And another relevant thing is like, for example, the uh the yellow flavor Mark and all of the scripts and stuff. It has like a very small subset of users. It's like Fred Walker on on the team and himanshu and Enrique uh on the content. Editor like they're the main ones using this right now. So when I'm doing big changes like reorganizing the files or the names or stuff I'll, just give them a heads up say like I, don't need a review, but this is just an informal.

C

Let you know this is happening and if I'm doing anything, you really don't like. Let me know foreign.

B

Yeah and and that communication can happen on the Mr or even in slack too, it was like those are, those are all helpful means of communication and that uh yeah I find a good way. um Sometimes going back, you get me kind of open a can of worms.

B

I have to close it to get people's attention when I was like I, don't want to assign it to a specific person, but I need some random person in a group um I'll often ping, the engineering manager of like hey, can someone on your team help out with this.

B

E

Not a bad idea because yeah or the PM or the engineering manager, someone that's like tied to uh like feature like completion. Everything.

B

Things that helps me triage, Community contributions, a lot easier because I just have to identify. What group is this touching and then, rather than me, picking the engineer I'm just going to pick the engineering manager and they'll be able to.

E

B

Definitely go on like just randomly pick. People.

A

D

Yeah nor I have a little delay so sometimes I I speak in uh over people. Sorry uh yeah in security. That's basically our process to like find the team that owns the feature and say hey empm. We have this, please schedule it so so yeah at least no.

A

D

Doing it there's an entire team that does that so.

B

Let me ask you a quick question Dominic and then and then and we're getting close to time. um I was here, berating the bystander effect, but that's kind of how Security reviews are handled, we're just paying the security group and someone from the security team hops on how do.

D

You all someone assigned uh every week, There's a person who's on rotation to handle all the abstract things. Every time I have seconds ping. We have a slack bot that posts in our Channel, so so either the personal rotation does the review or redirects it to the app stick engineer: who's a closest to that piece of code. Okay,.

B

Do they um is that all that they're focused on that week.

D

Is the main focus it is accepted that you won't get any deep work done that weekend? Okay,.

B

That's cool yeah, thanks for sharing that that makes a lot of sense all right! um Thanks everybody for the interesting discussion. It's.

C

A good one, yeah.

B

Yeah they're always good ones, but I guess this was a recent good one.

A

B

Catch you all next time, adios.

C