GitLab Code Review Weekly Workshop, 27 Jan 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Jan 27, 2023

Description

In this session we talk about diffing diffs, labeling MR's, and handling very cognitively complex MR's. We also pair up on a small MR review.

00:00 - Diffing diffs
16:13 - Labeling MR's
28:00 - Handling very cognitively complex MR's
37:32 - Pairing on a new feature flag MR

A

Hey welcome to the code of the weekly Workshop uh where we talked about code review. um I am gonna, share my screen, so we're looking at the same thing. I wanted to share one of my favorite things to do from time to time, which is different, diffs, which we've talked about a little bit, uh but sometimes I feel compelled to do this when ever in Mr is updated, so I'm scrolling to the bottom here and when an MR is updated.

B

A

Also rebased and so here's here's the really paranoid situation one could be in as a code. Reviewer is so. This was the new commit, but when there's a rebase, even though it says this is authored a week ago during the rebase process, these commits could still technically change. Now, it's not like I, don't trust Miguel, whatever, as often I just get a little OCD and I want to make sure uh and if I have time I'll be like okay. Let me just try to compare the versions now.

A

If I did, uh if I went to changes and I tried to compare version 17 with version 18., uh it's.

B

C

A

It pushed a bunch of changes for master, so I'm, not even gonna, let it load it's gonna, it's gonna blow up, uh so I can't do this from the UI. How do I tell for between one push and another push what actually changed.

B

Yeah, that's a great question: I um I, just don't check at that point because I didn't know, there was another way to check yeah.

A

Yeah, so uh this is where I open the terminal. So you know this is gonna, get real.

B

A

I'll, do is I'll, do that diffing and then two I forgot about how they call this, but you can like do multiple pipes into a command, so I'll pipe uh I'll type it out, but I'm going to do the first.

A

So this was the latest push, and then this was the push right before it and I'm. Looking for this, Commit This last commit here and so I want to pipe in the difference between origin master and support. You do three dots so that it does the merge difference between the two, the difference between the merge base of the branches and then I could do get diff origin Master with the latest Shaw or I, can actually now just curl the diff of that Mr and.

B

A

It you're now in them I like I'm, comfortable with Emily I'm, so I'm in them right now, there's other diffing tools. You probably could use them.

B

Dip like a default install on Max I.

A

Haven't yeah I.

B

Mean I have been I, don't know if it came with that ever if I installed it.

A

uh But I'll show you one of the reasons I use this and I really like it, um I'm sure that there's other ones uh but here's kind of what I'm looking for I'm looking for these, not where there's the same Plus on both sides, I'm looking for uh like here, we added something copy token, but then I left the suggestion of like hey. Maybe we should keep it consistent as copy registration. So that's something here's why I use ventif y uh here is. We were adding this block, but then I left the comment of oh.

A

Sometimes this can be falsies, so maybe we want to have an if all around it now for me to compare that, the two inner blocks are the same. I could just manually delete that outer tag, and now I see oh yeah. These are the same except for the white space that was wrapping it whatever. So that's my favorite things with them. Diff is it'll update the diff as I make changes to it. So if I really want to just compare two.

B

A

I can kind of just line those up and actually compare it with two blocks without reading it at.

B

All that's really nice I'm, like I, don't know, I feel like I'm like a 10 bam user I, don't know how to get in. There add stuff delete lines.

B

Yeah I, don't know, there's I know like delete a single line, I, don't I! You did some fancy keyboard thing that let you delete I.

A

Did the lines at once, yeah.

B

I can Google things to get me and I know how to get out of it, which is the most important.

A

B

Know how to get out um okay, but.

A

You could, and so with all that being said, like you could um I wonder how effective this would be with um diff fuel uh and so I could just read through this.

B

But it's not colorized like I.

A

There might be params I can pass to diff you to ads to make it colorized a bit, but I won't be able to get that like feedback. That I like doing of I, really want to compare these blocks that are now moved from each other and I can align them up. So I can actually uh confirm these are the same without uh actually reading the code so but yeah.

A

This might I'm sure that there's for for non-dem users there's another tool to use besides vimtiff, but that's what I use the main takeaway is the the piping of this commit which Now isn't attached to a branch but is still I, don't know, they're still available somehow and then I just pipe. The the latest Mr diff.

B

Well, I like it and you've got that commit from the command page.

A

No because this so this one I had to get it from the timeline, uh because if you go to the commits page, we did a rebase so.

B

A

I mean different: okay.

B

Interesting yeah.

A

So the main point is different: diffs when there's a rebase, how do you, how can you actually tell what's changed.

B

The rebase thing it really caused it has caused me problems when I'm trying to do comparison between what changed um between two like commits yeah. So then that's cool I.

A

Like it, uh yeah try, try them diff, it's it's! You know if you know how to get out of it. That's pretty good uh you'll have to add the exclamation mark at the end, because it may yell at you about yeah.

B

Yeah but I mean worst case. Two is I, can always um just close my terminal and then it's gone there. You go yeah.

A

Ctrl C is pretty good. Yeah activity monitors. That's next level.

B

Oh no I feel like when I run that thing I feel like I, look at it and it's like activity monitors taking a ton of your CPU and I'm like this is not what you should be doing. That's.

A

Really funny uh hey John, thanks for hopping on.

C

Hello I was trying to see if, if jetbrains can do deaths of arbitrary Shaws like that, that aren't on the I think I'm just nipping it with Master right yeah, if, like you or if you checked out the head of the branch. Well, no, if you checked out, like you, did the the previous sha before the rebase and then you said like get, you know, right click get compare with and then you put in the branch I think you would get the same effect in the the jet brain stiff, fear.

A

Does jet brains so what I'm using it is to actually diff two diffs? Well jet brains? Let you do that and if not what you can probably do is write one diff to a file and then write another diff to a file and.

C

Then yeah like you, can.

A

B

Really, oh you're muted gone.

A

C

There's it's not key bound, but if you do the command.

B

C

A minute I thought you're.

A

Hitting the oh, you know.

B

C

B

C

Shortcut you're.

B

Doing is also it's also uh yeah.

C

A

How convenient is that you know you, can you can mute yourself while, while diffing.

C

Wait: it didn't do it this time when I had it it's weird I. Do this all the time? That's so weird it like somehow Zoom grabbed the focus anyway command shift a which is also I. Never realized this also. The mute key binding silver is like it's like command, T and vs code. It's the search for command, okay, the command palette! If you type diff in there, you'll find open blank diff window and there you can pick any two arbitrary files or.

A

C

Can also just like I have a temp directory. You know I'm in most projects you have a temp directory, that's ignored, I'll just dump them in there and then you can highlight them both and do command D yeah.

A

That's a good idea.

C

I like that, but yeah, and then you get the nice diff viewer with all the coloring.

A

C

The three panes and everything I.

B

C

That brains, user right, I.

B

Am I'm trying I just I, try to learn new things every once in a while like, but a lot of the times, it's more like key bindings I installed that thing that you recommended it yells at me a lot but I try to learn some of them. Yeah.

C

I'm I the mode I'm in on that is like there's some things. I just want to use the mouse for, like the.

A

C

Like everything when I'm debugging, all of the the buttons there in the continuing skip I'm like because you have to sort of like get into that pain and then usually, you might use them like no like when I'm, when I'm in debugging mode, like my mind, just wants to click it yeah, it's the way, I think so. I've turned off all of the nags like no I'm I'm using the mouse for the debugger. Don't bug me about it anymore. That.

B

Makes sense, I haven't been great about using the debugger, so I'm hoping I saw someone ask a question about how theirs was set up and I saw that you commented on it. So I made it a reminder to go back and look at that thread because um so powerful.

C

Yeah, like me and Paul, like we watch this John Carmack, the guy who wrote Doom like arguably the most famous programmer ever and he did an interview with Lex Friedman and that's one of the things he was talking about, just like how powerful it is to to have your debugger and like the golang stuff and all of the things stuff I've been doing for remote development.

C

I, don't know, go at all like barely learning it and like that's. What I rely on the debugger I'm like what what is happening here like.

B

C

Know how to read variables and state and stuff like I, have no idea the details of a language, but I can make it do what I want. Just because I know here's the data and here's how I'm trying to manipulate it and then between that and chat GPT like asking it how I do something in goal: Langer, yeah.

B

Oh I never thought to ask chat. Gpt. Oh.

C

Man we rely on it so much Terry like it's.

B

I mean I'm in the same spot, with go like I'm, a maintainer for a go project and I'm like well.

C

At GPT, it's like it's. What do you think Paul? It's.

A

C

The web IDE right yeah.

A

It's our senior Council.

C

A

Gpt is the honorary editor anyway,.

B

I did sign up for it, but then um I don't know.

A

I know, but now you got to pay for it. Oh if.

B

You want, oh, you have to pay. Oh.

A

C

B

A terrible developer like I, don't like paying for things, I think it's not the development I, just only pay for anything. I.

A

Agree, no I'm with you Terry I and then you know you have this idealism and then people like stalman and Linus Torvalds fill your head with like yeah software should be free and that just for me that just buries me.

C

Opposite I'm, like is this marginally useful here, take my money, yeah I.

B

C

Started running for things.

B

I have but but I I.

A

Probably will pay for chat, GP team um because I found it so helpful. It was pretty amazing. It.

C

Is it's yeah, it's changed the way that and co-pilot I use a lot. Yeah yeah.

A

All the time it's changed the way I write code.

C

Like I, just like write a little bit of something and then I see what copilot completes and then I think like did you brand new newbie programmers I think is dangerous because they don't know anything but like for me. Learning golang I'm, like I know what I want to do. I just don't know how to do it, and in that case it's great. It's like here's something that may be wrong, but it's close enough to get me there, whereas I would have been Googling for half an hour.

B

Yeah and almost feel that I got when I was starting, which was like not to start with a framework when you're learning something start with like the language yeah.

C

B

Is you like the framework is using first so that you understand when things are broken um I guess you need to be able to understand when chat GPT is giving you maybe not correct advice? Yes, because.

A

It's not it's not it's very rarely 100 correct, but it's a great it's. It's a great.

B

A

Yeah uh the uh what was I gonna, say: I, don't remember: oh no um Source graph is coming out with an AI assistant tool.

B

A

That's going to be really exciting.

C

You know that terminal that I use uh warp I think I could.

A

C

It's got integration with with codex like with with one of the AI engines, and then there you just like uh type a comment. You know like a bashed out, comment pound space. How do I? You know how.

A

C

Numbers from one to ten and it'll write out the shell command for you to do that. I.

A

Would love it for it to like? You know right now, it's helpful, but then it Teeters off to like every every command ends up like curling, some shell from like it's just isn't doing. Pseudo curl, like all sorts of very dangerous things or anyways.

C

But uh debugger Terry, yes like it's.

B

I, have it on I? Have that channel favorited for like look at this later so feel.

C

Free like anytime, just set up. If you want me to help, you walk you through something or repair.

B

Yeah I definitely will yeah.

C

Anytime, let me know.

B

I am I was hoping I got like the Golan set up. Well, it was kind of a pain to set up because I wanted it to utilize. Asdf's version of going and I did get that working, but it took me a while yeah.

C

Mine mine seems to be working fine, at least for the get live agent code.

B

Mine, it didn't I just heard that there was any going installed at all and I had to like hack it, but it's fine I got it working I, don't know. I might I might take you up on that pairing session at some point, because I feel like I, went and like hacked some XML file to get it to register that there was a version of Google.

C

Support for ASDF, like in general, seems not.

B

C

Kind of shaky yeah.

B

A

That's interesting.

B

A

Asdf is interesting too, just what it does in general. Well, uh Terry. You had a question well.

B

I, guess that's more! So it's not about reviewing Mrs but as part of like when you're doing maintainer, things like we are supposed to make sure that these zimmars are labeled by type and subtype right, I'm, pretty sure I.

A

Guess so I I yeah.

B

I get I have noticed a lot of like missing subtypes and then it's just like do you because you get like a notification about well there's just so many chatty discussions that happen so I feel like sometimes they'll missed, but also it's like not blocking so like. Is it truly required?

B

um You know my thought has been if I go in and just either like look at the issue. If the issue has it make that match, that's the easiest way, but if it doesn't have the issue doesn't have it like you just like add it yourself. Do you add it and then, like Ping, the author, hey I, made this decision about your Mr. Like?

B

Can you validate it.

A

B

Mean actually just change it and I'm like well. This is clearly a refactor. You know if it's clearly like maintenance type, okay I can probably go in and do like a refactor but I'm like Eminem.

A

Well, things get tough for me, it gets really tough when Mrs are rarely ever one thing.

B

A

If it's a refactor, which also fixes a bug, what do you do and I? My understanding is that the maintenance is for maintenance only and if something goes beyond that any kind of user-facing change, it's either a bug or a feature.

B

That's not aware of that! Well,.

A

I have no idea I actually don't know why I think that yeah.

C

Eric was talking about Eric RPM was talking about these yesterday and he brought up the graphs like these basically Drive, the graphs. You know they're the three colors for each group and he said what they.

C

What they're looking for is like consistency like you know, if your group is doing generally the same number of bugs features and maintenance and that you're, even though it's sort of a soft suggestion like they'll, obviously want you to see, want to see groups doing mostly features but, like I, think that's useful, like looking at what is this data used for?

C

Well, they use it as a proxy to know you know our our groups, not thrashing wildly between doing all bugs and all maintenance and all features, and are they cranking out features so the because it's in the handbook they're supposed to be there. So me personally, I wouldn't have a problem like telling the uh the author to say: hey. You need to do this because the handbook says you need to do this, and this is a data that we gather. Here's the section in the handbook that Paul has up.

C

That tells you how to do it, and you don't have to take that on yourself. I, wouldn't think what do you think Paul.

A

um It depends like if it's so obvious, you know, hey just take a bias for action.

C

And Jamie yeah.

A

If it's not lip, it's less obvious. It does seem to me that.

A

Well, I, don't all right upkeeping efforts in catch-up corrective improvements that are not features nor bugs.

C

The thing that gets me is like um subsequent, like in future enhancement, the second paragraph there subsequent user-facing improvements that refine the initial NBC by adding additional capabilities, including good user value, usability and tests like to me, like all the stuff I did on the git Love Flavor and markdown, like none of it, was user facing, but it was all like in support of the content editor. Why.

A

Would it include tests.

C

Scripts right, so, if you look at it that way, it's like well, okay, if there's some feature being delivered and anything I'm doing is in support of that feature, then it's Feature work, which makes complete sense to me like.

B

Yeah the feature isn't just the code.

C

To implement it is everything that supports it.

B

Yeah I mean you're breaking an issue or whatever down into multiple Mrs. That Mr should be counted towards that feature. Work right.

C

So, in that case, what's maintenance, it's like something that is like it's a it's a time pass thing it's like. If this is not a feature that we have actively worked on for like in months, okay, then it's maintenance. Otherwise it's in support of some feature. Delivery, yeah.

C

Yeah, which, if it's technical debt, it's like well I, did part of this feature now today, I'm doing the other part of it. That's not technical debt. That's like I'm working on this feature, iteratively.

B

And our pipeline changes not tests, or is that, like specifically related because of a pipeline.

C

Fails pipeline, that is a specific one for.

A

That yeah, so that's maintenance.

C

That's pretty clear right, I.

A

I think test is misleading and I think that's correct.

C

I, don't know I take that's my favorite part of this whole thing. Yes, this is Feature work, because tests are part of the future like I, wouldn't deliver this feature without test would I no.

B

I feel like the test should be be there to support the changes that you're making, but if you're going in and like fixing a flaky spec, that's not like a feature enhancement.

C

Exactly right, because that features long done and gone, it's not active Feature work that is, maintenance.

A

Well, the enhance I, don't know yeah I I, don't know, I do think the tests- I, don't I, think that's erroneously here, but I won't I.

C

Guarantee you that whoever wrote the section did not put nearly a thought into it that we are right now.

B

C

B

Like it means like, it should include good user values ability, and it should include tests like this type of work. Not that, like it includes just tests but like the features that you are working on in an MR should include tests.

A

Well, yeah, but.

B

That's that's very.

A

True and but at times I mean that's a good point, because then.

A

Yeah we could, we could we're I forgot the word of this, but word need deep, like and.

B

I think poetic. How.

C

Many angels can dance on the head of a pen exactly that's.

A

uh That's where we're at, um but this is probably a good one, to save off, especially being in the um the as a merge request coach that needs to triage Community contributions. uh This is this is helpful um and, as I interpret this also on through the lens of what Chad was describing, it seems like maintenance is, if it it has to First, not be a feature or a bug.

A

So it's like if something is pretty close to a bug, we're going to give it a bug, something's, pretty close to a feature we're going to give it a feature, but then, if it's not, if you're definitely not a feature, a bug and we're doing just general maintenance stuff, that's uh that's where it.

C

Goes yeah infrastructure or things that hadn't been touched in Forever is sort of like like clearly common code, not related to any feature that has not been touched forever.

C

That's kind of my only things that fall into maintenance in my book and for our personal group I'm, like I'm, very liberal, with the future flag, and sometimes people will go in and change it or viewers, but I'm like the numbers, don't matter honestly, it's just when they look at our metrics and when and that's what Eric said when he was talking about this on the Retro yesterday, I think he's like they said. How are your numbers so stable across?

C

You know the editor group you're, your feature bar is just like the same size right through the middle, and you got some bugs and maintenance and he's like I literally never look at this graph. I do not care about this breast.

A

This naturally happened. That's a natural.

C

It's the same way like if you strictly tdd, like you, saw this at pivotal. If you do strict test driven development, you know almost all the time you hit about 90 test coverage just.

A

C

Really yeah, even if you never look at it,.

A

Yeah, that's interesting handbooks to find when something is a bug issues, foreign.

C

If you touch the file yeah.

A

You better you better test everything.

B

A

um You bet if, if a community contributor accidentally removes half the tests, it should still have a hundred percent.

C

A

C

Like that, even in the spike mode I'm doing a remote development, I'm still writing tests just to like help myself yeah, but it's so funny. Like I wrote this test I'm like yeah. This is this little algorithm, I tested it. The test is good and there's a bug in it, and there was a bug in my test and it.

A

Was still ah man, you gotta write tests for your tests? That's so that's what the integration tests are. I guess, that's funny the um getting to that high level of percentage. The way I I. Think of it is like with tdd there's tests that are really helpful during development, and that gets you really close. But then you just Harden everything else. So it's like whatever is not covered. We just write some hardening tests. I I, don't know how strict TD that is, but I know uh that test hardening idea is a that's an idea.

A

I read in a book some.

C

But my metaphor for like getting 100 coverage, which I think is a good goal. Some people argue that it's not it's like you. You've got a safety net right and it's like. Do you want your safety net to be like just as good as it needs to be, or do you want to have like every single strand there yeah.

A

Yeah, but you can't get over, you can't get over the top with it, but it's a good goal.

C

Yeah yeah 100 line coverage, not 100 Branch coverage, but line coverage like if you have enough branch that is not covered at all like you should have a test for that or just delete it right.

A

Or if your code.

C

Like ternary, or maybe something I.

A

Don't know right, but I mean if your code is having lots of branches. That's a pretty good sign of the difficulty of brand coverage implies.

A

C

Things up and there's the climatic complexity is too high. Yeah yeah, it's not cohesive enough.

A

Yep yeah well, uh is there any other code review uh topics, questions that we should discuss or are we thinking about pairing up on a code review.

B

I, don't have any more questions. I have a maybe thing we can pair upon that I just got assigned. Oh.

A

That'd be good.

B

Well, I, don't know when it was assigned to me, but.

C

B

C

You do when you feel, like you, can't wrap your head around uh NMR to correctly maintain a review and, like you, don't feel safe about it. You do pass it on and you just take your time or you that's.

A

A really good question.

C

You feel better.

A

uh What do you do? Terry.

B

Well, um I mean for the maintainer stuff. It's hard. I um have tried to adopt like Paul's advice is to like ask all the questions as I'm going through it that come to me and then go back and like see which ones do I want to keep all of them or not um I like to make sure that it's reviewed by someone like from that person's team.

B

That knows what's going on, because sometimes I do get some complex ones with, like maybe like lfs, where I don't know how that feature works, and so, if it's been reviewed by someone that, like knows the feature knows how they wanted it to work.

B

um I do my best to try to like to try to give it another review, thinking about like how it fits into like kitlab as a whole, but also I tend to focus more on. Like are we following all the coding standards?

B

um I, don't know if yeah I don't know. If it's really that complex I don't know if I've passed, I've passed, some off I didn't tell them. It was because too complex I'm, just like I, do not have time to like dig into this and.

C

It's really the.

B

Truth because it's like you, know, I get something and I'm like okay. I, don't have the time to dig into this right now, like I'm, just gonna pass this off to them. I'm.

A

Imagining you writing have.

C

Somebody in their group review it as well.

B

um I have before like, if it hasn't been I haven't run into too many, where you get something super complex that doesn't have someone that's like from the other person's group. That's reviewed. It.

C

What are we gonna say? Paul.

A

Oh I was gonna say that I. Imagine Terry. Writing a comment. I don't have time for all caps. Bold. This Mr table.

C

B

Just like I don't have the bandwidth for this well I.

C

B

It's like small changes, but if you go in and it's like, you know, 25 files and it's very complicated and I don't feel like I have a good view on it. I'm, probably just more prone to passing it off, and it's.

A

Really great to at least say, like there's a lot of complexity here that I'm not able to follow at just being able to say something about. This is not easy to follow. Yeah.

B

A

A great signal to communicate transparently, somehow.

C

um Has been so weird like almost all the stuff I worked on, git love is like been off in a foreigner or Greenfield and then now, like there's been no other back end. I guess there's Casio now, but he's sort of like often Pages there's been no other back-end engineer for like what going on in year.

A

C

Else to review my code when I write it on the team.

A

When, when I review, Mars I always check them out locally because I'm often changing them as I read it I read it and I change it to.

B

You did you check what you're suggesting, because I do that, sometimes, where I'm like I, want to see before I suggest this, if it will work yeah.

A

But if it's like, okay, you know you have to go through multiple containers to even touch this feature. If it's something like I might even be able to test this, um if I'm reading something I'm confused by it, a question in my head, often is: why isn't it like this and I'm sure there's reasons, but can here's here's the ideal if I was reading this? This is what I would imagine it would be laid out and I see it's not. Is there a reason? Why is this something we can work towards?

A

Is it something that we haven't considered, or we did consider so that I always check out code locally and if something isn't making sense, uh either a if it's something that doesn't make sense, because I don't understand why it was done this way or B if it just doesn't make sense because we're integrating with libraries and third parties, it's like all sorts of interfaces that I have no idea about.

A

My ask is: can we co-locate all that complexity, because if.

C

A

It's if it's some inherited complexity, because we're like needing to interact with some sort of Google API that needs to just live in one box and let's make everything else. Something.

C

Encapsulated behind an interface that.

A

C

Exposes what you want to do with it and test it? Yeah.

A

Yeah and that's a that's, a something: I remember: learning from Bob Martin, a long time, because anything that crosses the architectural boundary. Let's put that and co-locate that in one thing, because you'll likely want to mock that if you're doing tests and stuff.

C

Or whatever, and then you could do enemy unit tests, which it's I heard that term years ago, and it's a really powerful concept that same interface that you build around this.

C

Whatever external thing that you can mock from this direction, you can also write, what's called an enemy unit test which is okay, it's like an integration test, but it says I'm going to use this real thing through this interface in the way that I expect it, through my, you know, narrowly filtered interface and I'm, going to run this all the time in CI to ensure that this external API that this Library works, the way I expect it to oh yeah yeah, and what that gives you is like when you do a version upgrade of that thing.

C

Yes, then those enemy tests, You Know The Enemy, is a third-party thing. It could change at any time. It could break me at any time and when you upgrade it it's like okay, does it still fulfill what I need it to do, regardless of how it may have changed over this upgrade or for an external API? It's like is this API still returning the same thing: did they break it without even versioning their API yeah and.

A

C

It's like the natural boundary to be able to do that. If you, if you're strict about it, yeah.

A

And I think it just goes to show of you: don't have to understand everything but understanding. If there's things you don't understand, yeah, let's co-locate it and put in a box, and hopefully we can untangle everything else around it. But I love that idea. Oh my gosh I love the perspective of yeah. Our third party dependencies. Aren't our allies they're like enemies, we've made temporary peace with that we, the.

C

Problem is when, when the API is a fluid interface, where, like you get this thing and then you call something on that thing, that gives you another thing and then you get another thing: yeah.

A

C

Does not lend it it's almost impossible to use that approach, because you have to you have to violate the law of Demeter to work with the thing at all. Yeah.

A

That's that's an interesting point. It's a good question Jen. So I.

B

Don't know if we gave you a good answer, but I totally don't feel bad about passing things off and I've actually had maintainers pass off. Mrs from that I've made where they're like I, don't feel comfortable reviewing something that is backed by elasticsearch, um which is fine, um so our team does tend to if it's a very complex I try to use a maintainer for my own team.

B

There's a lot yeah just sometimes things are there's things that it can affect that unless you've worked directly with the global search like work that you wouldn't know to think of so um yeah.

A

B

Paul you make good point if something is too complex, I just need to be like hey. This is a little bit too complex, I, don't understand it. I don't want to review it. So I'm passing on.

A

Yeah um and I think everyone that contributes to gitlab. Doesn't they take that feedback seriously and I? Think people want need to hear that and I want when I put in ours that I'm like okay, we're having to solve a hard problem. So here's my here's, the crazy idea I had when I was running. Let's see if people like it yeah and oftentimes I'll get back of like this is so magical and weird I, don't like it and that's really good feedback to me. It's like okay, I.

C

Thought it was good.

B

A

A minute yeah: let's not do it and we need that feedback. That's really good I.

B

Also feel like doing those how to test instructions by um authors and if you self-review your stuff, it's easier to catch, whether something's, too complicated yeah, yeah I, don't see a lot of people doing the self review. But there are some folks who do and when I review their quote. I'm like thank you yeah, because they leave me little notes and then I don't have to think about things.

B

A

Know what you're thinking there's nothing to look here in this test file just.

B

Skip no, it's just like oh I had a question about that. Oh cool. They left a comment where I can now give that comment a thumbs up, because I've read it and I did have. It was like. Oh I did not like I appreciate you so um yeah.

A

That's what I left.

B

An MR that I got assigned to review and I would love to see if uh we are.

A

B

Some time we've been looking at it very briefly: it's not I just had a few questions, um cool and not so much.

A

About you want.

C

To share your screen, or you want me to.

B

Drive I can share okay. um Let me blow the um X up a little bit, though yeah my glasses are not my current prescription. So that's good for me. So this is a setup profile tabs in view not so crazy about this number here, but uh this is a front-end ticket and I feel like those tend to be larger.

B

um So it looks like we're in the process of migrating the tabs on the user profile from Hamilton view, it's going to be split across multiple issues in Omars, which they have an epic which is cool. This sets up the view app and the tabs don't render anything and there's an issue for each Tab and each tab will be built out individually in future Mrs and it's behind a feature flag.

B

um I will probably do this later, which is how to validate, but there's not really much other things. I typically go here and look um to see like what kind of commits they have left and I. Don't think this needs a change along. I am I, guess some of the stuff that I would ask for this.

B

um I might click through these things, but I'm here to review back-end code, so I'll probably just focus on that for this, um this is fine, I would not yeah. This is just like I would not um there's nothing here to be concerned about uh this. Diff looks pretty large, so I probably want to come back and look at this. It looks like maybe um they have added a feature flag that loads in everything.

C

B

Not gonna I guess this is future. It's going to Future load in this profile tab, but it's not doing anything yet. Maybe.

A

Just for fun, can we try to? Can we try it a side by side, this stuff um I, don't I'm, not optimistic about yeah I wish we were better at this. Is why I check all Mrs out locally is because most local diff tools are pretty good at telling you when something is just white space changes yeah, which.

B

Is hard yeah so again, this is um not displaying I think this is just all indented and it's not displaying yeah.

C

B

So that's probably wait.

A

This is wait. Can you go back down so sorry, scroll up, I'm I'm, getting a little thrown off with what's removed on like a line 168 versus 167.

A

like? Why is there this condition? Oh there there was always a condition there. Oh.

B

It's so hard to read all right, yeah I think it's just all shifted.

C

Red on the top left like, if they added, if they duplicated this blocked and one is View and one is Hamel like why is there? What was there on the left before like I, would expect to see like one red block on the left and two green blocks on the right for the two future flag branches. Well,.

A

It's because we're not doing any kind of smart wide space changing. Oh.

B

Maybe I should turn let's.

A

See I, don't I think that's still going to oh.

B

A

Well, I, guess it's better, but is it better I.

B

C

Know: ah okay, yeah more sense! Well,.

B

I guess this kind of makes more sense, but it's you can't really see I guess you just need to know that this is like shifted in.

C

That's why I was confused yeah.

A

That's that's really interesting. Okay,.

B

Yeah and the only thing that's a little confusing here- is they have the if up here and the unless but I mean I get what they're trying to do.

A

B

A

Be able to switch back to the white space one if that may throw you off I.

B

Don't remember if I might have turned that on I, don't know if I normally have it on I'm constantly going in here and changing things and so I'm just like. Oh, let me just see how it looks right to me. Okay, so this is new development default, enabled 59.

B

um I do normally at least come in and make sure that there was one instance where somebody had an issue here and it wasn't actually the rollout issue. It was something else, so um I do normally just validate that the numbers match, because it's possible that the thing is not right.

B

um So that looks good to me um this. We don't normally review um one thing that I didn't notice when I was looking at this. Very briefly is that um so this is really my question.

B

This is just not updating the test, but I guess in this instance. It's okay, because the code for all these Tabs are not it's not there right.

A

So I imagine yeah since we the feature flags are enabled by default and feature spec land yeah.

B

Bring into this with another conversion from Hamilton, View I don't have enough experience with those like. How do you like I guess? How can we make sure that these will be updated.

A

B

When yeah I just want to I, guess I guess so they have issues for each of these profile pages. But, like there's, nothing failing.

A

B

Like when someone goes to make the changes, for whatever would make this fail like? How do you know to go in and say, oh yeah, I stubbed, that to false.

A

This is a really good question: I, don't know, because what can happen is people implementing this Overlook the feature specs until we're moving the flag and then all of a sudden we see, oh.

B

All the things.

A

Bam yeah, so it's it's worth I would just leave a a note of like be sure to as a it looks like as they're creating and they have issues for each one of the pages.

B

um Maybe add a task to like they.

A

Need to add, which.

B

Feature like yeah, which feature test, needs to be updated. Yeah.

A

Or they need to have a task for each every file where we're doing a false. We need to also have a set it to true, and does this work as well and like we uh that's a good idea.

B

um I mean maybe it doesn't need to be part of the individual issues for each page. Maybe they could have like a feature spec issue where they go in and change everything like I, don't know so.

A

That could be a question is like do we have do we have a follow-up for something? Here's.

C

An idea like, instead of them just stepping into false and ignoring the future specs, they could go ahead and add another branch for true that asserts the current behavior, which is there's a view component here. That does absolutely nothing and then, when they start adding the real behavior that that will break and force them to revisit this feature. Spec yeah.

A

And I think that's a cool way of taking it too um either one of those it's.

B

I'll put a thing in here um and.

A

I liked I liked our way Chad and I had a fun interaction. This it was like two two to three years ago, um where we were I think effectively refactoring something there's a top menu right, but we we knew that the feature specs weren't going to change something like that like we knew the feature specs weren't going to change, but we wanted to set it up such that those features are going to be ready to go when the somehow we use pending, specs and stuff to like to like it was.

A

It was an interesting approach and I. Remember it being really helpful. We we had a context for every one of these files for like with feature flag on which feature flag offs and the one was on was going to have the same feature specs, but but it like decorated pending around it, because those weren't quite ready yet, but then, once it came to removing everything it was just it was. It was pretty.

C

A lot of updates when you remove the feature flag and didn't have the context anymore, yeah.

B

Yeah I really like the idea of setting it to true and making sure it doesn't do something, because then that means that will fail when they change it. Yeah.

A

B

That so much better, that is a great idea. There's.

A

Another gotcha here that I discovered with the broken master that that we ran into Chad uh uh when we were working on Saturday or something uh which is normal schedule. For me, a surprise Chad was on.

C

Normal for me, too,.

A

Oh okay, uh I'm just playing the uh some spec. The reason we the author knew to put these here is because they used the pipeline failures to dictate I need to throw these feature Flags, however, uh some specs are pending, or that might.

B

A

And once that gets unskipped because it's fixed, but now it's broken because we have this feature flag thing: we'll have a broken master. So.

C

How did that ever get unmaster like that should have been caught on some Mr when they, uh the.

A

Mr pipeline skipped that job, because somehow it was detected as flaky.

B

A

B

About that I saw the post about that that someone posted so I also ran into that um where a bug got through an MR that we reviewed and I'm like. How are these specs passing, because the specs are changed and I looked and I'm like these specs are wrong, like they should not pass, and then nothing was in the pipeline. The entire top level of the spec was skipped with an issue assigned to it and I'm like so.

B

We made changes to the spec, it's not even being run, and if you didn't run it locally, you wouldn't know that it wasn't being run and so I almost wish. There was some automation or pipeline. That's like they are flaky specs that this Mr touches like you should fix them, or at least be aware that this is skipped so that you can take a look at it. It's.

A

Worth at it's worth, introducing a um it's worth asking a question on slack: if would running, would adding the pipeline run all spec or something does that skip our skipping thing? What's really scary to me is: what's defined as flaky comes from some something outside of our code base, I think so.

C

A

That means is there's some change outside of the code base, which.

C

I was going to ask like it shouldn't be able to make it in an MR like that thing should be hooked to trigger, like you can't change that without NMR.

B

Or like if an MR touches a flaky Speck, however, it's detected to be flaky, it shouldn't be. It should always be run. Yeah.

A

B

A

Flags imply, we need to run all the our specs.

B

A

uh So that's something to think about, potentially even like jihu may have uh like. We may be inadvertently breaking their pipeline because they may have their own specs. We.

B

Have a new pipeline for them, but I feel like it's a lot of fail and mine's been failing a bunch, so I'm kind of like not sure what to do with that, but I'm glad that we now have a pipeline for it. It just feels like it's, maybe under under development still so yeah.

A

B

Thanks for looking through the Mr with me and uh I, really appreciate the suggestions, I'm Gonna Keep Those in the back of my mind for the next time, I see this feature flag thanks.

A

For bringing it up, Terry appreciate it all right, you all have a great rest of the day. Yep adios, bye.