GitLab Package Group, 22 Feb 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Package Registry MR reviews efficiency tips

Description

https://docs.gitlab.com/ee/development/code_review.html

A

Hello, I'm David from the package team and um today I want to discuss about how to make gitlab Mr reviews a bit more efficient um thanks for being with me today, Rod Mars and Dima, and so let's get started so I'm going to share my screen here this one.

A

So we do have some code review uh guidelines, um but I don't want to go over them. I mean they are documented, so you can just read the documentation. I want to provide additional tips to make Mr reviews a bit more efficient, um so the whole thing about Mr reviews.

A

It's really to see it as a as a match of ping, pong or tennis or well, whatever spot, with a ball and two players, and you really pass the ball between the players, so it will be a match or round between you and reviewers or you and maintainers.

A

What is uh super important is marking those moments when you pass the ball, when it's super clear that okay I'm I'm done with my part. Now it's your turn.

A

um You need to make sure that those moments are super clear and explicit so that there is no possible confusion um as a as a Emma. Also sometimes I receive um like hey uh he is here, is um some of my review like and then I receive some some comments and am I. Okay. Is this a complete review?

A

Do you want me to start working on that feedback? Do you want me to wait for the whole feedback? What do I do here, and so it creates some confusion and with confusion? uh Well, you will need more time to like resync with the visual or maintainer.

A

So it's pretty crucial to make sure that um when you pass the ball of the of the Mr review, it's pretty clear. How do you do that? It's pretty simple.

A

um There are two ways and I would advise to use the two ways, not one or the other, but the two so one way is assigning um the reviewer field so so that you pass the ball and you assign the team member here in the reviewer field.

A

um The other way is to post a comment pinging uh that with your uh like this, so the comment and the assign uh reviewer this way, it's clear that you are passing the DMR review and then it will get back to you and so on and so forth.

A

um So with that out of um out of our minds, there are several other things you can do to make the Mr creation and Mr review a bit faster. So the first one is really simple: the branch name um there is a feature in gitlab. That is, if you start your branch Name by an issue number when you then create DMR, all the labels will get automatically applied from the issue.

A

So um yeah you will certainly learn, or you already know that well juggling with labels. It's not the most um like interesting time to spend in GitHub, so the less I do with labels the better. So usually we I will use that all the time. I just take the issue number start. The branch with the um the issue number and gitlab will apply the labels automatically for me and then I will just adjust them if necessary.

A

um That's the first thing, um the other thing. uh uh Second, one um yeah something I use in all my Mrs and it's not really mandatory.

A

It's I will have a context section all the time, no matter how large is how large gmr is. If it is a small one, if it is a large one, I, don't care. I will have the context. In that context, I will link the issue and I will provide the it's like a summary of the issue so that reviewers can get okay. What why do we need this change? It's really explaining the. Why and the whole context around it.

A

um One example I made a change here on on world course for file. Uploads file plots are a bit complex at gitlab, so that's, okay, fine I will put the file upload schema or the the interactions in the context you have mermaid GS available to create those schemas, and so that's what I I put so that reviewers don't need to like pull the upload documentation, see how this works or put the the workers files and and see how this works.

A

It's like this is the summary of what is doing currently, and this is what these Mr wants to enable and well from uh like um high point of view, we can see that we are reducing the amount of interactions.

A

um It's it's like arming or giving reviewers, and in this case it's reviewers or maintainers with enough knowledge so that they understand all the changes properly.

A

um Usually, when you do your first Mrs, you will try to reach a domain expert.

A

um So in this case someone with the package registry knowledge, that's fine, but that's not always possible um at some point. We you will start be using uh where it is here, uh dangerbot or review or roulette. So those are like random suggestions based on capacity and those members like you can you can consider that they don't have a clue how to package registry works, so any change on the package of Industry. The context will immensely help, um no matter how large the change is.

A

The other thing why this is useful, um usually when I work on changes, I discover things or details or I, don't know like, um for example, Maven snapshots how they are organized, because it's a really specific way to organize packages for Maven snapshots versions and so I will mentally or write down notes on how these are organized but then well. My memory is not that weight and well paper is not that way too, like it's easy to lose it. So what I do with this context?

A

Is that I put my notes somewhere, digitally like it's on DMR directly, and this way they are there forever I can I can come back if I need a refresher like, let's say: okay, uh yeah I'm working on file uploads, oh yeah I had dsmr about changing file, uploads and I described how file uploads work. Okay, I can just open up and and just check this camera. No, actually not this one. This one check this one and oh yeah. They work like this yeah right.

A

So uh it's it's a way to like archive my notes, uh digitally um sometimes I I'm, no very very often I would say I even go further.

A

um It's not always the case, but if your changes are doing things like um things that might be surprising or things that might trigger a question, I try to anticipate those questions and I will just directly put a a comment on on DMR without even assigning reviewers like I. Don't care just preparing DML for for review.

A

um This is again to give more context more knowledge to understand what is happening and and well it's more work, because you need to like develop that skill to anticipate questions. But um at some point you will, you will be accurate, like you will receive reviewers saying: oh yeah. This was my question and you just answer it thanks and by doing that, I'm, avoiding one ping pong back like one ping pong from the reviewer, because the question was already answered.

A

um Yeah, if something is really really not super clear from the code and and Mr comment, that could be a flag that you need a code comment, because ultimately, the the code is what uh everyone will see, and so that could be also a flag. When you comment something really specific in the in the change that perhaps you need a code comment so that uh further you or other team members will be grateful to have that comment to understand what is going on.

A

Now the other thing I want to mention is yeah. Oh yeah, it's it's. When you comment on uh on the Mr, like you reply to feedback or you do something thanks, you can group all those comments and post them in a single update. If you want. This is really useful because they are saved um uh locally in your browser from not wrong yeah that that that's in your browser, which means that I could like say, comment one here and you start a review.

A

You see this comment is now pending um and for now it has not been posted, so I can go back to DMR. Well, perhaps you will not believe that it has been persisted, but if I open a new tab, we open DMR. My comment: is there? It's persisted so I would comment things blah blah blah comment to.

A

You add them to the review and you start stacking all those comments. um It's not only on the on the code side. You can also start. Let's say here: I want to reply this uh common three and you can uh to review uh the additional thing.

A

I do with this uh because usually um I publish my changes and the CI pipeline is running and then I I will reply to feedback like yeah I changed this and blah blah blah I will prepare the ping pong back with quick actions, so you can assign reviewers from here directly uh yeah, let's assign Teemo, uh you save the comment and that's it. This is pending now.

A

uh So um this is prepared why the CI pipeline is running and once it's green I can go back here and and just hit the the submit review it will just post all the comments assign the mother with your and that's it I'm I'm done like I'm I'm passing the ball um that that helps um I know well.

A

I saw some members like putting one comment posting it putting another comment: posting it putting another comment, posting it, and this creates some noise in the sense that all the participants that had that are here will receive one email notification per comment which is like um it's not wasting time, but it's creating noise because instead of receiving one email with all the comments, so that the reviewer is um the reviewer or the maintainer knows. Oh yeah I need to review this.

A

It will receive like one two, three, four five uh emails and, and they are not really valuable until the reviewer is properly assigned like he receives the the the ball and he has to do the the review. So I would suggest packing all the comments packing all the changes in the in DMR in one single review and just pause. The review when you are when you are done uh this. This helps in creating less noise and making things a bit more efficient.

A

Another thing when you assign um members for review, you need to be super clear on what aspect you want the review to be on.

A

um As you may know, we do have several aspects or layers or whatever you want to call them on on gitlab MLS. Usually they are detected by a danger board, and it's this thing category. So in this case, I had a backend, a change and the Workhorse change. Okay.

A

So it's like having two tracks of reviews, but that doesn't mean that you can do both of them at the same time. So when you assign um members as reviewers make sure that it's super explicit what aspect you want to review, uh you want them to review.

A

um It's not that obvious here, because back-end and workers they are like highly different code base, but you could have backend and database and we have members that are reviewer for backend and reviewer for database, and so, if you tell them hey, can you review this like this is not super explicit and they don't know if they need to check them out for database review or back-end review. uh It's it's more. It's better to be uh explicit!.

A

um Yeah and that's yeah, that's about uh the things I I do on the Mars, uh but yeah. Really the core is the the context section um yeah I know it seems that it's it's more work and that's that's true. It's more work like um instead of saying, hey check this issue, I'm I'm, like presenting the problem in the issue on uh as a summary, but the amount of time that I spend doing those or writing those context.

A

Section um I'm, pretty sure that it's nothing compared to the amount of ping, pongs and feedback uh I saved, which means um because your reviewer can be in a different time zone like you could be waiting for a full day to get a review. So if I save that ping pong I save one full day, and that's that that's a lot of time.

A

So that's why I will always write those context section all the time, no matter the situation. If it is a security fix, a production incident fix or something urgent I will. I will always take time to write those contexts. Sections foreign.

A

What else can help yeah the that's a section we we have how to set up and validate locally um as a back-end maintainer, no matter which MRI I'm looking but I will always test the Mr locally to make sure that it's working the way it is intended. So those instructions really help a lot.

A

um You will see in the package registry, you don't have always old commands or the all the package manager comments in in mind, so it really helps to have them written and then um not only for for the reviewers or maintainers, but also for yourself. Let's say that now this has been merged. I, don't know, deployed to let's say staging, which is here now. I need to go to staging and and verify this Mr. Well, what will I do well, usually I will just check the instructions, I just wrote and just follow the plan.

A

I just left so those instructions. In my eyes they have a like a double value. It's it's for reviewers that are interested in testing this. It's it's like a triple value. It's for maintainers because well in my mind, back and maintainer should test changes locally, but when possible, unless there is some exception and and third it's also for yourself when you want to verify this on staging or production, because at some point you will be juggling with many Mrs. So you are working on mr1.

A

Then you you push that into review and you are working on MR2 mr1. You get some feedback, you go back and then you go back to MR2 and then suddenly mr1 is merged. You're still working on MR2 mr1 is deployed, and now you need to verify mr1, but your mind is on MR2, so like switching to mr1 and finding those instructions on how to verify this can take time- and this is this, what what you have in this section is my shortcut like I.

A

Don't need to think about it's just I, follow the instructions and and check the behavior on staging of production depends on the change.

A

um Yeah, that's uh pretty much it. um The bottom line is uh yeah. It's it's more work, you're, presenting more data, more information in DMR, but in the long run it it really helps you to get faster reviews.

A

I really saw a change when I started doing those contexts, um sections as a backend maintainer, it's also like sometimes I I would receive an MR and, like the explanation, is, is really like check this issue.

A

um Okay, so I will open the issue, but on the issue there is a discussion between the engineer, the um the the product manager and so I need to collect all the information to know. Okay, what is needed here? What what we want to do and- and you need to like gather all the discussions- and this is time that I need to invest so that I can provide a good review on on that email.

A

um That's uh what I want, what what I try to avoid with those context, sections like everything is packed in DMR. You don't need to open an issue like everything is explained there and- and it's you, you have all the all the marbles to understand what is going on here.

A

A

uh Yeah I think that's it talk about quick actions.

A

Yeah I think I, don't have anything else. Do you have any questions.

A

So either everything was clear or everything was not clear at all which one it is.

B

Yeah it's clear, I'm yeah I have like one question: I I think I read about um uh review apps. That should be ready when we push merge requests so I'm, not sure if there is some deployed version of our code in Standalone application that we can verify our changes on or not.

A

um Yeah uh there is one I think it's uh manual. Let me check it's a manual uh job in the pipeline, so you open the pipeline uh yeah and towards the end of the pipeline.

A

There is this one start review up pipeline and this one will start the the review up and then once it is started, um you will have a link somewhere.

A

I, don't recall correctly, I think it might be just below the pipeline status. You will have a link on hey. This has been deployed on this review app.

B

Okay, so it's a manual job we, we should start it manually to.

A

Yeah um I would say that I only do I only use those jobs and I think they are automatic for those Mrs. If I have a documentation, changes uh because documentation, it's one thing uh working with the markdown file and it's another thing to see it within the the gitlab documentation template. You see, we have side menus, we have some uh I, don't know if there is yeah some like we can Mark some sections with a specific marking, so that hey this is a node or this is a warning.

A

This is a an information and you you might want to check those changes. uh What is the the actual result for backend changes.

A

No I will not verify them in the review app um because I'm I will verify them on staging directly I'm, not sure how the review app is set up. If there is some data um but uh since like, if you check I I would check very thoroughly the the change locally, and so, if that change is working locally, with the amount of testing that I do locally, it's like I have a high confidence that it will be the same on staging uh if it's not the same on staging.

A

That means that data is having an impact on Behavior, meaning that the data on staging is somehow um changing the the change Behavior, which could be an edge case or or something but usually yeah for backend changes. I will not use the review app.

B

Okay, yeah thanks yeah and maybe another question uh yeah, and how how much should I wait till, for example, I remind a reviewer that I still waiting for yeah so.

A

We do have an SLO for this uh uh and I think it's on this page. No.

A

A

uh I yeah I know that it's somewhere but.

B

A

Here, um if I'm, not wrong, I think it's two or three days.

A

So I always wait three days uh to being a really well.

A

um The other thing is that I will never ping someone on slack directly unless it's a exceptional situation, but otherwise I will not being a team member on slack directly for an MR. Are we just being on DML directly with a with a comment like hey, um just making sure that this is on your radar or or something?

A

um What could happen well and, and it happens, um members will forgot, we will forget about their PTO, and so when, like when dangerbot runs, it will check the gitlab status, which is the one you can set here um like edit status, and so there are some things. I think it's in the code review I just saw the the icons yeah, that's the one.

A

So if you are on a on a PTO, um you should have a string like PTO or oo or the palm tree emoji, so that danger knows that you are not available, but well members will forget about that, and so that could happen that uh yeah um like, for example, I, might be in this member, and actually this member is on PTO.

A

So he will like never reply back, so I will bring back after three days and on the fourth day, I will check with other means to see, if he's on PTO or not um on slack, there is a like the tool we use to input our ptos. You can also ask or search for a team member and check that team member, ptos and trees so that you know if that member is on a PTO or not, and usually that tool will also update the slack statues of.

B

A

Member, so just checking Slack might give you a hint of a member being on video um yeah that that that happened and will happen because well sometimes you like uh go on PTO and forget about setting up your gitlab status and and everything.

B

Yeah, thank you.

A

Are there any other questions.

A

All right then, um well I, guess we can stop here and um well thanks everyone for attending and thanks for those watching the recording until the end and well see you on the next one. Bye.