GitLab Source Code Group, 18 Apr 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Owners architecture

Description

Code Owners architecture walkthrough

A

B

So take away the code owners walk through. Please, let me share my screen.

A

All right, you see that.

A

Yep, okay, so I'm gonna give a walk through of the architecture of code owners, um I recently added this documentation page.

A

um That gives a overview of how the how the logic works and the entry points into that logic and stuff. So um you can see this page. It's already live on the on the doc site, so it's already linked to you from the feature page as well. So from this feature page, you can see in the Technical Resources or of course you can search for it.

A

um So in here we've got a few things: we've got them a bit of details about business logic, details about the related models and the controllers and services which um interact with that business logic and some diagrams here that show the flow um of how we enter different pieces of the code owners, business logic. So I go over the business logic, so we have a few files code. Owners um is an Enterprise only feature. So all of this lives in the ee directory It's all under um lib gitlab.

A

There is a class in the gitlab folder code owners and then all of the rest are names based within the code owner's namespace. So we have the main module which is used to interact with code owners um in the lib gitlab directory, and we have a file called code owners loader, which will load the correct file from the repo. So we can request a code owner's file from the default Branch or we can pass in a reference and we can fetch the file from you know different branch or tag.

A

We also have some say, maybe easier to use the it out of order. So we have the file, which is a class which wraps the code owner's file and exposes some useful public methods and within that we also have entries which are not sure if you're all familiar with how code owners Works, maybe I, should actually open your stocks. So you can see the example so.

B

Just to interrupt real quickly, Joe, and so this could possibly be being viewed by other people as well, so yeah I hope it would be awesome. Thank you.

A

Yes, so um encode owners, you know we have, you, don't actually have to use sections. You can do if you want to group rules logically, but the these rows are the entries. So the entry is a path followed by a list of group, subgroups or users that own the file, um and by specifying this it essentially means that you have to get approval from this.

A

uh This one looks like a group, so if you modify a file within the ee slash docs directory, then you need to request a approval from the docs group, some from a member of the docs group, so those are entries where have I just lost and faced it. Okay. So here we go so yeah, so we have the file and entries that wrap those.

A

There has actually been another class um introduced since I've added this documentation, which hasn't hasn't been included in here yet, which is a section so the sections are, you know we have the section name here, there's also some other stuff. You can do sections. One of them is documented here, which is a default um owner, so this would mean the development section is by default. It's owned by the dev team. So if you don't add any owners after the path, then it will be assigned to the dev team.

A

However, if you add an owner, then it's a rise of evil, um so there's also a section class to to wrap around that logic now as well, which which will extract you know the name and whether the section is optional, how many um approvals it needs and stuff like that right.

A

So we also have so part of the low level, so the loader class will um first load the file and wrap it and then it'll wrap it in this this file class and then it will extract all of the entries and it will also load um all of the owners. So if we have any users or groups specified as owners it'll do that in a single database query or a.

B

A

Database queries but it'll try to optimize it as as well as it can. um So we have these two classes, the user loader and the groups loader, and we also have a reference extractor. So this reference extractor is responsible for figuring out kind of find a good example here. So when you have multiple so like this one right, so you have multiple references here. So we have a reference to group the group with nested subgroups so that the second one here is a subgroup within group, so the reference extractor will will basically just pass it.

A

This whole screen that I've just highlighted and then the reference extractor will pass that and spit back out. Group and group witnessed itself group.

A

And, interestingly, you can actually use emails as well, which I wasn't aware of until this morning.

A

Right so and then one last piece is validator, so I can only feel like this is potentially in the wrong place. um Validator is actually used when we, let me show you the flowchart, so this is easier to understand. So there are two different entry points for doing code owner checks.

A

So the first one happens when somebody pushes code to gitlab and we have a class called get access and within that we do some checks based on if you're, pushing or pulling if you're pushing it performs um checks on the changes that are being pushed, and one of those checks is a diff check, so it finds all of the um files that have been changed and it it validates a few things. So um this is the diff check class, so we can see here. Let me just enlarge that maybe easier to see.

A

So this is the ee version in the in the non-ee version. There's some other checks as well, I think, um but in here we validate the code owners. So we basically just call the code owner validator with the um the project, the graph Branch name and the paths that have been changed and the validator class will. Then um it's going to check if brands the branch requires code owner approval, so this is actually um part of the protected branches feature.

A

So when you protect a branch, you can select a checkbox that says require code owner approval for any of the files in here, and then it's going to use the code owner loader to find all the entries for the div and it's going to check. If, if any of the files in the code owner file has been pushed without um being approved so being pushed directly to a protected Branch, it's then going to return an error, so this is actually from your command line interface right.

A

So when you do git purchase, it's actually going to return. This error message. It's going to say, pushes to collected branches that contain files, and it's going to tell you the path and it's going to you know essentially tell you that you can't do that.

A

um So currently I I do think this validator should potentially be moved out of the decode owner namespace and maybe into the diff um the the checks namespace. Let's get log checks, dish, diff, checked, I think it should maybe maybe belonging this name first, um because we're going to be introducing other validators as well to ensure that the code owner file itself is actually valid and I think that that logic, you know, belongs in the code and namespace rather than this um okay. So let's go back to this flow chart.

A

So that's the the first way that we access code owners. So when you're pushing you know, we check if any of the files are changed, that you're not supposed to the second way that we use code owners is on merge requests. So um when you create a motorquest, you update a match, request um or there's some other other stuff.

A

So if you create protected branches, there's a create service which is going to go through all of the most requests that are already open and make sure the rules are applied and um there's a merge request, refresh service and a Reload merge request head diff service. On top of my head, I'm not exactly sure what they do, but they're related they're all related to merge, requests right. So when an action happens within a merge request or you create a new new protected Branch we're going to trigger um the merge request.

A

Sync code owner approval rules, so sync code owner approval rules.

A

Essentially what this is going to do is create merge, request approval rules based on the code owners file for that much request. So the way this works is, for example, say that I I'm, creating a merge request against.

A

um So the target branch is feature, slash, something and I've I've branched off that and I'm, adding a small change to that, so that the first thing it's going to do is load the code owners file from the target branch and we're going to find all of the entries that are related to the merge request itself. So here we've got code owner entries, so this is going to call the the main code owners file and it's going to call entries for merge, request we're going to pass in the merge request and then the merge request.

A

Diff is the previous div, so what that's going to do is find all of the entries in the code owner file that apply to files that have been changed within the um the merge request itself and we're going to first um try to delete any rules that might be outdated. So if this merge request is not new, um this code would have been processed once already. So the first thing we're going to do is delete any rules that no longer apply.

A

So, for example, if I have a code learner entry that protects the readme file and I create a merge request and I change the readme, and then somebody reviews it, and it says that you don't need to add this to the readme. I might go back in and remove that change, sort of readme and then the second time I push, because the readme file is no longer in the merge request. We're gonna we're gonna need to delete that that approval rule or we're going to have an approval rule that's invalid.

A

So that's the first thing we're doing here, which so we're going to call motorquest approval rules and then on the approval rules model. We have a method, not matching pattern and we're going to pass in patterns. So patterns is taken from the code owner entries, so we're just going to take all of the code entries so remembering these are only the entries that are specific to the files in the major ghost and we're going to map the patterns.

A

So we're going to.

C

A

All of these I'm going to delete all and then after we've done that we're going to try and create new approval rules that have been added so.

A

That's here so create rule, we're going to do approval, merge, request, rule find or create by code owner rule and merge request and then entry. So entry is going to be each entry from the code on a file if I just find out what this actually called from.

A

So you see here we're getting the rules by pattern by section, so it's taking a rule from each section and then we're going to go over each codon entry and create a merge request, approval rule for that for that entry. So that means that when you're approving a merger question not actually interacting directly with the code owner file, you are actually you know, interacting with merge, request approval rules that mimic what's been defined in the code owners file.

A

Right so think, that's actually.

A

I got the files open here, but so yeah I recommend poking around in this. It's quite interesting how the code Works um we can see the file here. This is the class that wraps the actual um code owner file and we have a loader. So the loader you pass in you call the entries it's going to burst um it's going to. Where is it load in this file? So it does it here. So we're going to wrap the file in the code.

A

The code is blob in a file class and then we're going to pass that file and load the entries for the paths, so the paths we we passed in ourselves like when we have a merge request. We pass in order password from the merge request, so we just load what we need to load and once we've loaded those entries, we then load the users and groups that are required.

A

And yeah, then this is the reference extractation. You see it's also fairly simple um yeah, so I think that's that's probably all of it. So if anyone has any questions, look at that time.

A

Yeah I think I covered everything.

A

A

B

um I didn't want to be first, but um if anyone else has a question, please go ahead of me.

B

Okay, so with the, um if you go back to where it's doing the merge request, approval rules so there's a couple of there was a couple of things. One is so for just to completely understand this. So for every merge request.

B

We have entries in the database that are effectively um the um the calculation of of the code owner rules against this particular depending on what files are in that merge request all.

A

Right, so if there's.

B

No file, if there's no files that are protected by code owners there'll be nothing in those tables. For example,.

A

B

A

May be different different approval rules, but not not code or no approval rules.

B

Oh okay, so just mix it's interming with other approvals. Okay, so I just it just it I mean I'm, not obviously that close to how that looks, but um it does seem, is it effective? Is it inefficient to um to be creating? You know millions of rows. Well, let me ask the question another way: had we designed code owners in a different way, would it be possible to to just create a simple set of rows in in tables that could be then joined by whatever merger quests are coming along.

A

A

I mean one way to do. It would obviously be reading directly from the code owners file. um That would obviously be quite inefficient because you'll be calling guestley quite regularly yeah um it's possible. We may have been able to do it in a different way, where and so I think.

B

Yeah there was, there was a proposal to to move code editors to not not to move it but to to store it to get to the database right.

A

Yeah um that was yeah to store the actual phone, so I think we. So we spoke recently about potentially removing um at the moment when you create a merge request where you load the code owners file from the target branch, um which means that there are at one point in time. There are multiple code owners files that live in every rebound, because each branch technically has its own codons file, um whether that change they changed or not. That's not really relevant right, but it would mean we'd need to keep track of them.

A

So if you create a merge request, you know against a branch that is not the default Branch. We would be loading a completely separate code owns file.

A

um However, if we remove that we only use the one from the default, Branch I think it there would be a possibility for only having um you know for having a record of the rules in the default Branch at this present time, and then you could um use, join, joins and queries to check those on merge requests which may be more efficient, um but I think you know. We we've discussed this and looked at this recently and I.

A

Think that's not really possible at this point in time, because it you know it is a feature that we want to maintain, allowing people to manage multiple programs files. So.

B

Yeah, well also might be a big change to something I'm, just wondering why we may have gone down this path in the first place: I guess what I'm thinking? What I'm focusing on is that we I guess we may have tens of millions or more rows.

A

Oh yeah I'm sure we do yeah.

B

And um yeah and so I guess the other kind of related question is so you said something earlier on. At least sorry, unless someone else has a question: does anyone else have a question.

A

B

Answer so in that case, I'll ask a second question, which is um code owners you mentioned uh earlier on that um on the branch protection rule we say it's protected by code owners, so this codeone is only coming to effect. If we have that turned on because I thought it was, you know Branch.

A

Rules, no so there's two there's two separate ways that it's used, so the the one that you're you're mentioning now is um when you create a protected Branch, say: I protect, feature, slash star, so that would say any branch that starts with feature slash um protector and when you create that protected Branch, you can turn the checkbox on that says, require code owner approval so that what that means is you can't push files directly to that branch that are defined in the code owners file?

A

So if I created a branch code, feature slash my feature and I try to push the readme, um it would give me an error, and the only way for me to actually change that file in that Branch would be to First create a different branch that was not named feature slash, my feature so maybe call it addition to my feature and I'd have to do a merge request into my Branch. So that's the first way that we use code owners and that's specifically related to protected branches.

A

But then the the other way is enabled by default, which is you know, protecting merge requests, um so all merge requests.

A

If you have a code ends file, all most requests get these approval rules um and, additionally, you can actually turn on another type of approval rule which says allow any eligible approval to approve this and code people that are defined within code owners also pastors eligible approvers, um so that that means, if you you know, if you just Define, if you said, read me as owned by the doc team, then you push a change to read me and you have the all that all eligible approvers approval all turned on. That means.

A

The doc team can approve that merge, request.

B

So yeah, so with those approvals, then again, if anyone's got a question, just jump in um or comment, um they turn into um a required approval in the.

A

Virtual approval so that yeah most of the time there are some situations where approvals are optional. Obviously, you can actually specify that they're optional within the code owners file, but there's also situations around group.

C

A

Where it's it's not possible to um make a file, approval requires like it. If a if a group is inherited rather than a direct member of a project at the moment, I think it's not possible to make that. um Let me just check your documentation because I know there's some well.

B

I see if we look at you know the gitlab project and we have lots of code owners, but when we do a merger Quest it doesn't get rounded to those people necessarily or does it. um It just gets.

A

Router I mean it doesn't get, it doesn't get routed to them, but it does say you you, somebody within the code owners has to approve this.

B

So, does that mean that every maintainer is basically a code owner? Okay,.

A

B

A

Actually I think I think that is correct. Yeah, okay,.

A

Okay, well, I think the the multiple I mean. Maybe we can have a look at.

B

um So can we get into a situation where um a merger request can't be approved because of a malfunctioning, or you know invalid, in some way code noticeable.

A

No, so in the future, that might be the case, but for now what happens is if a difficult an entry doesn't have any. So if you could actually Define an entry with code owners say, for example, if I said at some random username that doesn't really exist, yeah exact.

C

Code in this file.

A

Would technically be valid, however, that user doesn't exist in the gitlab. You know database. So what currently happens is the approval. Rule is created on the merge request, but it's automatically approved.

A

um However, we've been discussing this and you know potentially in the future. We may change that um so that yeah, that's an interesting creature, yeah exactly yeah. Well, this there's a thing like I think originally. When we wrote this, we were saying we don't want it to block. You know if there's.

C

An issue with it: we don't want to block approval, so we don't want.

A

To prevent approvals from happening, um but I think now we're you know we're switching more into okay. This is like a security feature, so now we're looking at this, like. Maybe it shouldn't be approved, and you know somebody that is responsible for the code and as far as really go and fix that problem.

C

A

B

C

B

C

Ahead I mean: do you know what the big loophole is right there and.

A

C

It exists the docs team, the let me just drop in one drop in a merge request here. Yeah that'd be good whatever you want to search.

A

C

Because the the tech writing team is an exception to the the auditor rules where or approval and merge because they realized what we do for a living. We make changes to other people's merge requests and not allowing someone who edits like me to make my edits and then merge. The word means we have to bring in yet another person.

C

We fought very hard for this loophole. So if you need a use case for for when things go wrong, hi I'm, your liberal.

A

So so, just as we clear the loophole is uh docs don't need a the approval is optional. Is that.

C

A

C

Okay, because what we did, we showed them that quite a few of the top contributors to get lab are, in fact, the tech writing team and yeah yeah.

B

But but don't we have optional approvals? Okay,.

A

Yeah yeah: well, oh, that's the news. I think that's what Amy's, referring to as yeah am.

B

I, the only person without a cat.

C

No I don't know the.

B

C

Here's the internet.

C

Anyway, we can talk about this more separately, but yeah I, I I knew that I knew the edge case as soon as you brought it up.

A

Yeah, this is actually one of the examples of what you're just asking Sean. So when.

C

A

Have a rule is invalid. You can see here. The invalid rules are automatically approved to unblock the merge request. Yeah.

B

It is, it is an interesting approach, um because yeah so I understand the reasoning behind that, but um at the same time, yeah we've created a rule, okay, so the rules invalid. But then we automatically improve it. That's it seems uh well, I, guess that's what we're doing so. I haven't seen any issues around that actually, no.

A

B

Noticed any so I guess no.

C

One's complaining about it, oh sweetie, we hear about it multiple times a week. I just haven't.

B

C

You on it yeah.

B

Yeah I mean you would imagine, maybe there should be. You know an administrator approval or you know some other high level approval to approve, or at least make this Behavior optional, opt-in, but I. Guess it's not it's not opt-in and um that's what we have. But um okay.

B

Anyone else got comments or questions, because remember anyone can be working on these features in upcoming milestones.

B

I think everyone has. Everyone touched co-owners at some point now hands up. If you haven't worked on code owners, yet.

B

Okay, Patrick, you get the next issue.

B

Jerry, have you worked on code owners? I? Have no okay cool.

B

um And then just about General quality of code Joe I mean what are your feelings on the code quality and the spec quality.

A

Yeah I mean it's pretty good, I think that actually not 100 related to code owners, but the diff checks. Logic is a little bit messy I think it could be improved a bit but code like code owners is fairly small and self-contained at the moment. So I think the the co-pilot is pretty good there and we had quite good test coverage as well.

B

It's interesting what you mentioned about the validation step being where it is. We are actually a time, but um because you know, one of the upcoming features is to have a way of validating code owners, potentially On Demand by a user. So that would require us, on top of internal re-architecture, to make that possible.

A

Yeah yeah well I mean not necessarily, we I mean we could just rename this file to diff check, valid data or something I just feel like it potentially doesn't really need to to live here.

B

I said we could more or less expose this logic through an API and have it on demand that.

A

Would this would get a specific? This is different logic right. So this is, um this validator I think is kind of like a is named slightly wrong like so it's actually it's not validating the code owners itself right. So it's actually validating changes that have been pushed against the code: okay, okay, it's approving or yeah kind of yeah, something like that.

B

Okay, so we're actually at time does anyone else have any anything else to add.

B

I have added two points to the agenda, but uh those are just read-only.