GitLab Code Review Weekly Workshop, 25 Nov 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Nov 25, 2022

Description

In this session we pair review on a community MR. Thanks for contributing to GitLab!

A

Hey so welcome to the code review weekly Workshop Yannick Scott, a Mr that we will I think we're gonna pair reveal on. um So let's check it out. uh If you want to share your screen, Yanni.

B

Absolutely let me press this button. There yeah you're supposed what yes I, see it awesome, so um yeah disclaimer um I just got assigned to it. I, don't know anything about it. Just yet: hey Andre hello, um perfect time to for you to join just to quickly pick you up, um I've just been inside the community contribution. I, don't know nothing about it yet and we'll be looking into it and just carrying on it. So this is where we currently at nice sweet.

B

So um a new visibility setting for the package registry was added to the product settings behind the package feature.

A

So there's there's one for sure way to review it and, to you know catch any issues is to just merge it and if.

C

A

People create issues so um yeah. You can just have the users review. It.

B

Interesting like that, would that would that would end this meeting rather quickly. I could just press two buttons and we'll be done. I'm.

A

Not sure it sounds like that falls in lines with our efficiency values: I'm joking I'm, joking I'm,.

B

Not sure if it scales to be honest,.

A

I, don't think um including our users in the code revealing process without their permission is probably what they're? It's not what they're looking for! No.

C

But that's what Google does all the time with all.

A

Their you know, capture yeah. This is a good point.

C

B

um So here we are here's our keyboard, there's some art redesigns the package registry settings within the project settings still behind the yada feature: flag: okay,.

A

So it looks like previously, there was some sort of drop down.

C

B

Yeah definitely do agree, and to me it's oh, it actually seems like that. This is um well redesign is one thing, but also kind of the logic might have changed, because we're now having these these nested kind of talks over there yep.

A

So now there's not a drop down which is interesting because I think my if I remember this page, there is always a drop down here um like, as you can say. Oh, we want merge, requests to be enabled for the project, but only project members can use merge, requests or something like that.

B

A

Oh sorry, I was I was gonna say it does make a lot of sense. What they're trying to do is splitting up the.

A

The package registry versus anyone can pull from it like, so we want anyone to be able to pull from it. I guess, but only certainly previously was was. It was only project members that could push to it right.

B

Everyone with access everyone, everyone with access, does not seem like a um like a good option to have in a drop down kind of I'd say so um it is there. We there we go this. This feels much better. um Everyone.

B

Public project, okay, I, get it um first question. That's that's coming to my mind, um not to be mean about this, but should we uh ah okay ux already proved awesome. That's um that was what I've been a question of myself sweet. So it's only about the changes in here. Awesome.

A

What is Ezekiel, are you have done? What did he.

B

Say um he mentioned something about the um about the readability of the tests.

B

um That was the only thing I looked into, but before I'd say, let's give it a shot and see if we can reproduce um in the meantime, I've been enabling the feature Flags so in. uh Let me just duplicate this to another screen, so I can drive on the exact same okay same one.

B

So what does it say? Opportunity Decay go to in a project whatever before.

A

Looking at the code, you kind of want to recreate it. First, that's.

B

How I usually do things? What about you folks.

A

I told you I usually just merge it and let users review it. Gotcha, gotcha, okay,.

B

Yeah I, usually I, usually recreate, um because, basically, because is it if it doesn't work or if I see any problems while recreating then I already know what to look for, we probably have to get like. If it doesn't work, then I probably don't care too much about test coverage or something like. Let's get it get it working first and then we'll see about.

A

That makes sense and and um yeah.

B

um Visibility project features permissions there we go um package registry yeah there, it is speed and this is on a public project which this is um so we don't see the uh okay gotcha.

A

um We would probably expect to see that little drop down the nested one.

B

um Well, the the DMR kind of kind of has this.

A

Cover, if it's a public project, it.

B

Doesn't exactly let's quickly see if I can find any kind of private one on my GDK um I usually go public for the things I create.

A

um ah There we go I, see I see so, if you're ah so you're, the package registry could go beyond your project, visibility, I'm, guessing that's the situation. Anyone can pull packages with a package manager. Api, ah that's interesting.

A

Okay, it does look like they're doing. Documentation is separate. Mr I saw that in passing in a comment, um but yes this so.

B

Just to get this right, um so if we toggle this off, like a private project, is a private project. But um if this kind of builds out and for example, npm module I would still be able from a private project to access this via the npm. Several sets or any other package manager.

A

um You will be able to access this Project's package registry.

C

A

Yes, even if it's a project and that setting is enabled that's my understanding of um pretty cool.

B

A

Yeah yeah I mean so we can we can take, but it looks like it's just redesigning that back in logic sounds like has already been there. So.

B

This is designing.

A

The front and dead or I guess.

D

A

I think, looking at that, the the weirdest situation is, we have the nested check box checked the parent checks box is unchecked, so is that behaving as we would expect cool ah so fancy? Is this automatically say things so like? If you save it is? Is it.

B

So it's not automatically! You have to hit the button yeah.

A

You got to hit save and yeah. Can we go ahead and hit with it turned off like that? Can we hit save with it turned off like that? Okay,.

A

That was weird right.

B

She was successfully updated, um I, don't know, successfully update it.

A

B

A

Feel like we just discovered something you.

B

Know like I'm running into this too many times in our platform, because what I just did was nothing but reloading and I bet you it's now in the correct state, still broken.

C

A

C

A

This sounds sounds uh like it's not working all.

B

Right, okay, so this is on uh on a private project. Yeah.

C

You will try it on the public. One.

B

Yep, this is the public one.

A

um And, and by public it's oh I, see this.

B

Is also not it's not very yeah, that's weird. Why.

A

Does it go all the way down? That's that's. That's!.

B

Weird too, okay, but probably out of scope, I'm turning it off saving um was successfully updated. Checking in on the turn on um reloaded expected it to still be uh on you. You know what um let me just quickly um turn off the feature flag.

A

Can we check to like me here's situations happening either, it's not updating in the database or it is of dating in the database, and we always show it as on to start with.

A

I don't see if, if there's any other possible scenario,.

D

A

We confirm that it is updating in the database with a feature flag on or off it's it's fine. We can keep, we can keep manually testing it gotcha.

B

Do you do you want to see the decode diff or the network tab.

A

I kind of want to see the rails console man, oh.

B

There you go, um what do you want to look at.

A

uh I guess we want to find the Project based on the project ID, and we want to look for that project. Setting. Oh.

B

That's great you, you don't have any idea how much of a new I am on the latest console so.

A

So we can jump to the um I think at the Project's homepage. We can find the project ID.

B

um Before we do that, just a second since I'm still here, let me because the feature flag should now be updated. I'm, sorry to oh.

A

B

um Monitor is it gun, oh, the feature flag actually triggers this. The whole toggle.

A

D

Didn't we have it previously.

B

Well, the the screenshots say that this this should be there anything.

A

A

All right we gotta look at the code. What's going on, we gotta look at the code, but.

B

Okay, let me let me just quickly make two two notes about this. So um saving is an issue and displaying things when the flatters off is an issue: okay, um where it's console I'm. Sorry,.

A

Well now, I want to look at the code because I'm I'm.

B

Back at you, okay, there we go. um So what do we have going on.

A

ah It's a view app. Okay, this all makes sense. Now. Does it oh yeah, you know.

D

A

Were using something stable like react: I'm, joking I'm, sorry, everybody.

B

um Okay, first thing that that jumped at me was like: okay, we are updating um specs in in Ruby land, but don't have any changes in there, um but this just seems to.

A

Yeah, let's, let's jump inside Vector.

B

So that's all right.

A

um Okay, so that's just adding some constants. If you jump down that's adding some constants to keep going down, we got rid of feature access level options. Okay, keep going down registry enabled if registry access label level is greater than the feature access level, not.

B

Enabled um this is kind of hard to tell from Context, like are those computed values or what are we looking at here?.

A

Let's look at the template: I've I usually like to do that.

B

A

Of this file here.

B

uh um I, usually don't use Source control on vs code so help.

A

Me out, what do I need to look I think on the third icon from the top that one yeah.

B

There you go okay, let's.

A

Just click on the diff one at the top this settings panel that view.

C

B

You just click on just click, the file name. Yes,.

A

Thank you, I'm, sorry, ah okay, there we go. Yes, all right. Let's go down to the template. I like to see the the template, change I feel like I feel like there's.

B

Something changes.

A

B

View file holy crap, okay, so.

C

I wasn't lines of codes. Oh my God.

A

But this is Project settings yeah. Is it really a thousand lines of code? If you have, you know the same code over and over again yeah.

B

No, it's just a need for reflection.

A

I'm sorry I'm, like extra snarky right now, I'm really sorry, everybody all right. uh So we have a setting rail and we added. We got rid of this. ah We had a project feature setting component which we're not using in this place anymore and instead we're using a talk straight up. Just the toggle.

A

D

D

Value label label position: change I'd, be curious about this. Regarding DT back and things.

D

B

Is the second toggle okay, so this is probably checking if the if the project is private or not? That's what I expected to see in here.

A

Yes, that thing on line nine um 904, which we've removed I.

C

A

Going to be related to why this isn't saving on line 904.

C

Yeah, without have another name for the jail toggle. Maybe that's the reason.

A

Yeah, that's a good. It's so is name not even a property of GL toggle.

A

That makes sense too um well.

B

That would be a problem that would have already been in here before, just before the changes like name was already set, but it was a different component.

C

Yeah, it was a different component, it was Project features.

B

ah Gotcha, okay, okay and well. We can well um storybook real Target. Isn't it that's foreign.

B

um Props name: there's.

A

Everything here can we change it. How come.

B

A

um That's that's strange right. Well,.

B

There is some kind of a cold play ground or something in here isn't there that.

A

Means I think I think all of those are the knobs you're supposed to be able to change yeah you're.

B

Right I can't change everybody. Everything.

A

Can we um can we go back to the um to the GDK, and maybe we can inspect the input if it shows up it's not showing up at all.

D

B

Is that yeah, um so what.

A

About the flag off, it's not showing up at all is that is that what we're experiencing?

A

B

uh It's now called packages.

A

B

um Let me double check no.

A

B

It's a different thing.

B

A

Can we go back to the code.

C

B

Just on the editor.

C

A

Yes, India's code is nice.

A

So there's a VF of package registry access level enabled and packages available in line um 916 of the current file.

A

um Did we change those at all.

B

This has been touched.

A

For half a year um did we change the implementation of those computers or datas, or can we look at the references to these references in the file.

B

This is what you're looking for a lot of patches package registry.

A

ah Of course it's like yeah, so we we, yes all right.

B

um Really because it's not a yes for me, um like I what I'm seeing is I get this, so this is obviously a Target, whether it's checking if the flag is on or not because otherwise, then just wouldn't show anything but yeah.

A

So this isn't a news. Yes, this is now making sense. It's not a new flag. This is an old feature flag and we only show this when the feature flag is on. So yes, you turn the flag off. It's not going to be this.

B

Okay, so the whole feature is behind the slick.

B

Okay, so the whole package regulatory feature has been behind this flag and behind this flag, we've actually been redesigning stuff. So, yes, see this I would actually have to enable the flag and checkbox master yeah. That makes sense yeah yeah, oh okay, glad we got that sorted yeah, that's close um flexingly doesn't doesn't matter still saving and um saving, and what about the name for this year? Let's talk about well this. It seems that this is kind.

A

Of yeah, so let's, let's turn on the flag. Let's and can we turn on the flag and um uh Anna had a good idea of of seeing if the name is even being applied, I just assumed it was. Maybe it's not even being applied, but if it is being applied, I'm pretty sure it's the wrong name um and that's going to be related. Where would we go see that in.

B

The uh at least.

A

Let's just update the feature flag and then and then we can just inspect the element on the browser.

C

A

You keep exiting console like real fast. You like jump in you're like get me out of here. As soon as you update the feature flag, you don't want to hang out in rails. Land I, don't even know why it's.

B

Part of my work uh workflow, but I can't like it's just out of whatever um so I. um That's right. Let's see geotome! Oh.

A

I would just say about the.

D

Element like you could.

C

A

This way, or so name is a prop, but let's actually inspect the element and see if it's being applied to like some sort of input field. Can we do that.

B

Nice spawn input name there we go.

A

Input, yes, cool, yes, so there is some sort of hidden input. That's being said here great, but I'm pretty sure the name is not correct, because if you look at one of the other names um like, if we go to let's, let's look at the other rail real fast.

A

Can we go back to the inspect element and see the row above this one.

D

A

See, there's this like uh the parents to that input we have highlighted, is Project feature row if we look at just one of the siblings above that one, if you go down just a little bit.

B

um Down down on the in the Dom tree, yeah.

A

Like right, there right there, it's great Grace, yeah sweet, so see how that input name has like all of the project, wrappings and stuff.

B

um I, don't see any input name.

A

I know and I'm I'm, um there's lots of there's lots of text here and I'm trying to uh it's like five end lines down from the top of the thing of yeah. If you go like five elements up from what the gray highlighted element on your inspector.

A

D

Right I think we.

C

A

It anyways yeah. We definitely want to update the name, let's go for it.

D

B

Oh okay, um what do we want to do? That's, let's try to make more sense of all of this in the editor, um so the name uh gotcha. It is now package registry enabled and it has been brought.

B

And it has actually been what is.

B

um There's something I don't understand like this is a string name like it is. It is written as like a student accessing a two-dimensional array does not have a color in front of it. So just.

A

C

A

That's because it's that's because it's a it's a rails, form field, name man, but.

B

That's how we do it kinda.

A

Rails, land yeah, so what it's saying is like for the project model for the project models project, feature attributes for updating this access level. So we get all these names for free. When you we use Hamels form, Builder kind of thing, um we will say: hey we're, building a form for project project, feature attributes. So when we specify these names, these names are automatically going to match up.

A

But when we move forms like this from Hamilton View yeah, we get names that look kind of weird like this, but this the name that name formatting is a is not a JavaScript thing. This is this is how the rail is back. Kind of gonna expect the names and it's.

B

But it's it's mentioned, but it's obviously has some dependency or is important in a way because otherwise it's still be working and it's currently isn't.

A

Anything exactly yes, yes, so I'm, not sure what the name actually should be, because.

D

A

A

Sorry, as I see this.

A

Yeah so, um and- and we kind of spent some time reviewing this Mr so now we're at the point where we can decide how much of this problem do we want to solve ourselves and how much of the problem do we want to describe in volley over to the Mr author.

B

That might be a fair point like now giving him a good explanation of like do you. This is the problem we're running in um before before we actually started to fully review this. Maybe can you reproduce this and um get a final fix for it.

D

B

Might be interesting to do now right.

A

I think what I'm looking at this too from we made a front-end change and we changed one of the field names I, don't think we have a back-end property matching the field name matching these field names. It looks like previously we had just one setting, but now we have two inputs and I, don't think they're gonna they're not matching up to that one setting. So if I understand Our intention with this change, is we weren't really planning on changing the back end?

A

We were just changing how we submitted that one setting so I think we're going to want to introduce some sort of like hidden input. These toggles actually shouldn't have names and they should be setting the value for the hidden input. The hidden input has the old name. Does that make sense at all.

B

um Yeah, it does um assuming that when you say these inputs, you are referring to the new ones, we're just introducing correct.

A

Yeah I I think the intent.

B

A

Author's intention here might have been to not give names to these at all, but that we need to sink some sort of value to a hidden input of using the old name. I think that.

D

A

um So yeah, let's leave that yeah on line 927 sounds.

B

um Oh okay, so that is probably going to be a question. Yeah.

A

C

A

Because, like it is broken right.

B

I'm, a friendly guy, it's Friday I'll have questions you.

A

Don't feel like, is it not friendly to point out issues is that is that a generally frowned upon.

B

It it is, but I just think like. Why is this broken? It's a very valid question as well.

A

Got me you got me okay, so.

B

um What have you put in here? um Just a run, yeah I found.

B

The input does not say it's stayed to the back and is that.

C

A

Yep yeah, if you wanted to you, can leave a like a like a video of anytime I, create an issue comment: I, usually try to leave screenshots or a video of something, but you don't have to do that. um 100.

B

um Something I don't want to deal with now like getting OBS that come around screenshotting all at the same time, and that might be a little much but um I, don't know the video um we could. We could shoot or actually um whatever you feel like. um I would also be good with moving on to another topic, is um this finding um already helped me a lot in other other confidence to to phrase this question on my own, but I'll still take you along the way, whatever you feel like yeah.

A

I would the one thing I would add here is um and I would like to know if, if you would include something like this in this comment, I I like to suggest what might be wrong and I would ask like.

A

Are there back-end properties that match these names and if not, do we need to create a new hidden input with the old backend properties? Syncing up these values, like that's my that's I as I interpreted the situation, I think that's where we're gonna find ourselves um the other. The other thing that stood out to me, which we had an address here, is how come the old project feature. Setting component didn't work here, um because that seems when you have an a thousand line component, that's duplicating itself things get weird when it stops duplicating itself.

A

uh You know those are that's. That's the scary thing about duplicate code, I.

B

Haven't I haven't fully understood the issue you were just describing, so what you've been saying is like how has this been working in the in the previous version or what's the question.

A

You look at the rest of this component. We use that component. That's referenced on the removed line, 900 project feature setting, we use it all over the place, but for some reason here we couldn't use it.

B

ah Gotcha, okay, I see I see. Why are we not re reusing project feature setting component this time? Basically,.

A

That that's something I don't know. If that's just a question we can ask, or is that something we need to figure out um as we review this ourselves too, um we just I I haven't if I was reviewing. This I would look into I.

C

Think it was replaced because uh this whole label drop down whatever is intake, is a part of this component probably, and they wanted to change it to this nested toggles.

A

Yeah and and yeah that makes sense.

B

um So am I hearing this correctly, like project feature setting, is not supporting any kind of nested targets. I.

A

See options I, I, see, I, see what you're saying now it's like on line 9 64 of the the new files like we would specify options every time we do that and yeah. Clearly we're not doing options here. That makes sense.

B

That options would probably render, as this kind of thing, yeah I.

A

Assume? Okay, that makes sense. So that's not really a question like it makes sense why we want to get rid of it.

B

Sure, okay, so our main question is about the D naming and what do we need to do to actually get the front end to to speak the the same language or communicate in a working way with Rebecca.

C

A

Yeah cool yeah yeah, that's that was fun, find.

B

Awesome sweet um I'll, make sure to to provide a demo video and leave a comment. But thanks for.

B

Giving me some great space of yours, This was. um This was good right here, cool.

A

Well, uh yeah yeah we'll get some time. Does anyone have any interesting uh code review experience that they would like to share or have an MRI that they would like to pair on now or we can end early, sir you're all you're always close.

A

All right, I think I think by that the people have spoken by not speaking and gotcha. You all have a great um of the weekend uh and I guess rest of the weekend have a great weekend um and I'll see you all next time, thanks for hopping on.

C

D