GitLab GitLab Hackathon, 6 Jan 2021

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: GitLab Runner Open Office Hour

Description

This call is part of our quarterly GitLab Hackathon - https://about.gitlab.com/community/hackathon/

First reviewing MR: https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/2582
Second Reviewing MR: https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/2597

A

Right going live, I guess we're live hello! Everyone good morning afternoon evening, um I'm christas a senior called contributor program manager and with me I have steve like an engineer for the running program.

A

um We are on the beginning, let's say of our hackathon here. It's great to have you all with us. Welcome welcome just um some logistics uh reminder. Let me just briefly share my screen. Didn't.

A

All right, can you see my screen? It has a weird. um Let me change the orientation of the screen, the size of the screen, great, so um just a quick reminder of the call of conduct um we take the um here at gitlab. We take the safe and inclusivity of our event seriously. So just um just a brief reminder that all the events that we run are under the community code of contact that can be found on the link here on the slider.

A

It's really important for us to feel for everyone to feel safe and inclusive, and we won't tolerate any inappropriate behaviors. So please, if you have or face one or you know this one please reach out to us at contact gitlab.com.

A

uh We keep an eye on all our communication channels and merge requests on the channels as well so um yeah, but if we miss something or if you something just um let us know uh earlier today, we began with our hackathon kickoff. It was just a brief introduction of the hackathon. How the day is gonna go.

A

The two days are gonna go, and this is the first of the five sessions, the rest of the five sections that we have scheduled for today and tomorrow it's uh the runner office hour on the hackathon on the hackathon page. You can find more information about the hackathon, for example, the number of the merger quests that have already been submitted, which are 25 yay 37. Sorry, this is amazing, um if you do, if you're new at gitlab into the products as well feel free to check out the contribute page.

A

The contribute page consists a bunch of information on how to contribute specifically about development, which is the hackathon about it's all the necessary information about finding issues and also setting up the development environment. A special highlight in the new addition, which is the git pod.

A

We recently added a new section about how to set up your development environment with gitpod, which makes you set up the environment really.

B

A

And really fast and yeah. Also in back to the hackathon page, you can find the prices as well for your contributions and the sessions for the tutorials you can find. If you click on the times, you can find your local time zoom link to join and also the names and information of the speakers.

A

If you need any help, we have the the contributors guilty room. It's a jitter channel where all the community members, the core team within gitlab and also gitlab members, are, are there. So we can provide real-time support in case that you need anything. It's also bridged with gitlab's internal slack room room yeah. So there is. There is also the opportunity for other members to join whenever needed, so yeah welcome, and with that I will stop setting my screen and I will pass it over uh steve.

C

Thank you so much. uh Thank you for that kind introduction. um So these officers are usually very very freeform from our end. It will be either answering or asking answering questions from the community. I see some people already joined our call, so if they have any questions or anything they want to discuss, this is the time to do it.

C

We can discuss issues, merge, requests or anything like that, and then, if we still have time- and there are no answers- no questions anymore- I will choose some merch quests that are opened by the wider community and I'll do some live code review. uh The point for that is mostly to uh show the other side of what a code review looks like from from a maintainer perspective, so um there's no like barrier between the community contributor and us, and it's a bit more transparent on what goes on and to reviewing uh code review.

C

So I see three people in this zoom call. Do you have any questions or merge requests that you want me to review at the moment, feel free to speak.

C

A

Akita benson or mar do you have any questions or any requests any merge requests that you would be interested in exploring. Oh, there is a comment from benson.

A

C

Okay, if you don't mind me, asking benson, is there anything specific that you're looking at the merge process or just the code review in general or like how to open up a merge request with gitlab runner for.

C

Example: okay go to review in general, um so if there is no any other, if there isn't any other questions, I can get started with one of them that I have in mind and uh we'll go from there. um You, when I'm doing code review, uh feel free to jump in at any point in time. Ask questions um um or like say uh something else. So I don't mind.

C

So let me share my screen down and we can get started.

C

So uh the merge request I had in mind is um merch press two, two five, a two and this one is mostly uh follow-up, merge request that we had from uh the same community contributor um where this is talking about the kubernetes executor. So if you're not familiar, kettleberry supports uh multiple executions infrastructure, so one of them is stock. Another one is kubernetes, so we run your ci job inside of kubernetes cluster uh and one one of the things that we've been working on at the moment is adding support for host aliases.

C

So if you are not familiar.

C

With gitlab ci supports services, so imagine that your application needs access to a database right.

C

That means that the job that is running your test, that is connecting to the database is starting services right. So it's starting, for example, postgres database mysql database whatever, and then your tests need to communicate with this database right over the network. Now I won't go into too much technical details, but basically we need to have um um here. Is these keywords? The uh yes keywords where you can specify uh give me a second to find a good example on this. This.

A

One can you also, please mean a bit more, I'm checking the youtube live, and apparently the quality is not that great. So if you can just zoom a bit, it will be really helpful. Thank you.

C

Awesome uh thank you for that. So you can specify alice's right. You can either communicate with the postgres uh via the my postgres address or the db postgres right. It is mostly like uh convenience, sick um and there's also host aliases, so I'm not sure if you ever try to modify your etc host directory.

C

You might point a certain ip address to a domain manually, so it's like an internal dns server, uh it's very, very useful for like internal applications and so on and so forth. So we did this in a previous merger press. But when we were doing this, we found a new where we were always adding the localhost uh ip address. So it's 12 0, 0 1..

C

There are all the time, no matter. If you had services defined in your gitlab, cim and file or not, which is it wasn't, causing any problems, but it shouldn't be therapist while it might cause problems in the future and though it's like somewhat of an unexpected behavior right. So the main goal here is to remove us to only add this host alias inside of atc hosts when we actually needed to um and I'll show you an example of what we're trying to solve here.

C

So I have a simple job that is going to run a sleep command right and I have a gitlab runner instance here. So let me start it from scratch.

A

Can you please uh paste the merge request link here to the top that's out, maybe so I can distribute it. Thank you.

C

Let me do that so.

C

That is the image requested.

C

Now um so I started the runner locally, and this is just the development I have. The gitlab runner code base checked out locally and I can just run the main go binary main file, and now I can see that I have zero port at the moment running.

C

I will start the job so as soon as I will start the job, a new pod will be created inside of my kubernetes cluster and we can inspect some things from it right. So.

C

I have pause running and I'm gonna do.

C

Cat ptc hosts and you can see uh entries added by host addresses this is kubernetes feature and then uh we're just saying port one. Two, oh twelve, one two seven point: zero zero one right, which is not what we want we want. We only want to add this when we actually need it right, and this is how it works when we have a service defined. So let me go.

C

Here and let me start a pipeline that has a service, so we see how it actually.

C

C

I'll give it a sleep of two.

B

C

Now that it's running, I should see another part and it's still a container creating, let's give it a.

C

C

Okay, so it's just pulling me out just here: that's why it's taking a bit longer so.

C

It's running and I'm just gonna cut the edc host files again and I should see the following, so it added the localhost and the host analysis right. So when you ping db, it will bring the localhost and the coop energies will run out everything for you to the database service right so um to rear. To reiterate, we don't want to add this when we don't have any services or we don't need it at all right.

C

So uh this is what this community contribution is trying to achieve and we'll take a look at it together. um So you saw me that I got. I got all the context.

C

I tried it manually, we're producing it manually, and this is like one of the um uh first steps I do when I do a code review just because I want to understand what the problem is and where the problem relies before actually reviewing the code, because if I'm just reviewing code- and I don't really know what the problem is- it's mostly doing reviewing of the changes instead of actual actual code review, of what we need to do to achieve this program right.

C

So this is touching two files here, so host aliases and hostilis is test, so it is already like a good indication that we're both modifying application logic and we're adding tests to test to actually check the application. Logic is valid.

C

So, let's let me open up the file in my editor as well, so we get a better understanding.

C

And we're modifying mostly, I think, the create surface of solus. So let's go there.

C

And we have, this is what we have on the master branch at the moment. So just a quick rundown. um Actually, I won't run through it manually, um just because it's a lot of context driven so I'll check the branch locally.

C

So we have an. I need, git, alias where I do get mr origin and then the id of the merge request on the actual branch, and this one will actually check out that merge request into my local environment, and we can see that changes are here.

C

So, let's start looking at the diff, what changed here and then we'll uh we'll test it out as well so host aliases before we always used to have the local host alice here and probably this is where the issue was happening right. We were just defaulting to the local host host, alias and like done just appending to it.

C

Whenever we need to which is not what we want, we only want to append it or add it when we actually meet it right, so we're creating an empty slice here and then here we are host aliases host name and alice. So, where is this host name being defined and where is this alias being defined? Let me check it out here so host name.

B

Okay, yeah host names.

C

It's just appending to it the offers and then it's adding the alias. So this one is for services and hostnames and then it's literally just uh appending to the host names when we actually need it. If it's valid for uh there's a specification for dns, it's called 1123., uh we validate it and then, if it's valid we add the alias and the service values as well, and then we do it at the bottom here um if we have any aliases.

C

So if linked hostname, if we don't have any hostname and just return and return, nothing right, so we're not returning the localhost host to localhost anymore and then we're returning the default. One here seems to be what we need right. So, let's test it out now, so I'm running on it environment.

C

And let's run a new.

C

C

If we check our cluster, we should have a part running.

C

Check the host files here and we can see that we no longer have the empty host statuses, which is what we want right now.

C

I think this check is fine, but give me a second. If I yeah it seems fine right, so we're just checking if there's no hostname, if there's no hostname just return earlier right, which.

B

Is what we want.

C

Here so this is good and now, let's move to the next file- and this is where we're actually adding tests so no hostname analysis when servers only with parts, so first before doing that, actually, let's check where it's adding it, because sometimes um everyone even me- I do it like we end up adding tests to uh cases to test where it's not doesn't really belong to. So this is online 76.

C

And this one's here, so this is for the test, create just create host services. Software perfect, so we're creating this new host value system only with ports.

B

C

So this is adding a case where there is a service defined, but it has a port to define, and this is uh for the check in this one. So we go to host statuses, we're checking if this uh the services imports now the ports are not really useful for the user. These are mostly for uh web ide settings, so gitlab itself is a web ide and then you can define certain services, so, for example, you can have file syncing and so on and so forth.

C

I will not go into details into that one, but if it does not, if it has a port, we don't want an alias to it basically.

C

So this is what it's checking here: uh we're still defining a service, but we're defining a port for it, and we want to make sure that there are no no host aliases right.

C

And then we're also adding another one here: uh hostile uses with no services, so the user specifies their own host aliases inside of their config tunnel file, but they didn't define any any uh any services right. So we don't. We only expect this one. So I see two um two improvements that we can make, so the first one would be. um We are not really like. Well, I'm just looking at this.

C

If I'm just coming through this, I'm not really seeing what is the expected result right now, uh if you're familiar with go, um if you don't define a variable, if you don't define a value to the variable, it will use the zero value and in this case, for a slice it isn't on this slice always so since we never defined it, it was asserting.

A

C

An empty slice! So if we look at the usage of expected.

B

C

Here uh we're just checking that the host values we got from create host values and the expected one are the same, so basically we're doing a quality check right.

C

So I think it's good to be explicit here in this case, just because, if I'm just reading through the uh test cases, I'm saying okay, uh if there's only a service defined with port, I expect no host alexis right. So, instead of not having it it's it's a bit, you have to know what's going on in the test, so that is one suggestion I will suggest right now.

C

C

Let me write that quickly.

C

C

B

A

C

Usually like to play suggestions or show the diff of it. So um if someone do the change for the user, but also I'm very clear of what I'm expecting or what I think it should look like just so, it prevents a lot of back and forth and things like that, and sometimes it's easier to explain code with code instead of just trying to explain it in english right so I'll. Do this and then the community contributor will see this one and either d agree or disagree, and then we'll have a discussion.

C

uh Now this one hosts aliases with no service. uh I have feeling we're already somewhat testing this somewhere else. So, um let's.

B

Look at all the test cases.

C

That we have, um we have aliases.

B

Normal services defined.

C

And we're getting these aliases, we have a service and a service with the port and we're getting this hostile uses with no service and.

B

Host solutions.

C

With service, what else.

C

Let's see all of these okay.

C

So one other test we can have is no services or aliases defined right. So what if we just do this.

C

So services is uh empty.

B

C

Hostilities is empty as well right, so, if both of these empty, we expect the expected services to be empty as well.

C

So if we run this specific case.

C

It should work.

C

And worse, it's not working, because there is something actually in it.

C

C

So let me check that for a second.

C

I wonder why that is so: let's debug.

C

C

B

It's returning.

C

On top, that's why I think, let me check well, if that's the case.

C

Yeah and it's good that we tested it because.

A

C

Soon, as I see nil, I start thinking. Oh will this cause a panic somewhere and go because if we try to reference something that is nil, it might cause a panic. So let me check if that would would be the case right.

C

So let me call check for everything that is calling create host alice's.

C

And we're calling it the test and we're calling it an the kubernetes executor called get hostilities.

C

And host aliases is being and positive prepared.config and, let's see where the source status is being used, it's being used to construct the api request to the kubernetes executor.

C

um So this should be fine. That is fine actually, so that is expected. So here we're being super super expensive, explicit right. If no surfaces are defined and no aliases are defined, we're gonna end up returning and now um I'll add it.

C

Here now, one word scenario I see is like we here we're returning an empty slice, and here we're returning a note right is that something that we should be bothered about, or not um at the moment. I'm not sure um this is where I usually just either think about it. A bit like.

B

C

I don't know, try to make a decision within an hour or so or like spend some time thinking about it just to see if it's actually a good idea or not um or try to fix it and see if that's a something that we should do so that let's see why it's returning unknown here, so we forgot to host alice's and, let's add the debug breakpoint here and run to and debug this specific test case.

C

So we're calling create host alice's, create service audiences, and here we go to.

C

Nil uh all the whole slides. This is.

B

C

So we won't go into it, and host solutions is zero, so it will just append nothing to it and all her silences are still you know, and this because when we use for here um it's using the nil value here. But if we, for example, change this into.

C

Those aliases and do something like this, so we're defining an empty struct, we'll see something differently right. So if we just run this test like.

C

This, I will see that's done. This will work instead, opening.

C

Okay, but if you look at my editor, it started yelling.

B

C

Me a bit and it's called empty slice declaration for violator now I've I'll apply the hint that's basically doing this now. I won't go into detail why this needs to be done this way, but what I will do is I will give you a link that explains this. uh This is like an internal detail about go. If we go on the wiki and look at the code review.

B

Comments, where is it.

C

um No, it's slice, let's start declaring empty slices here, so I'll leave this in the chat and basically.

C

uh Sometimes like, for example, when we're encoding json, which in this case we actually are right, um we have to remember that kettlebell is talking to kubernetes over the api and the api is written in json, it's a json api, so we send it json and it gives us json um and when we're declaring declaring an empty slice, we should always prefer the var keyword instead of this one um since it's empty uh uh it creates and then slice and stuff um there's less memory allocation and so on and so forth, and things like that.

C

So um in this case, that is fine now, so this is what we want now. What I'm curious about is if we can get this to know as well like. Why is this return? Actually, this should be named as well. Let me check.

C

Yeah, I was wrong here and these are like normal rabbit holes. I end up going down to when doing code reviews just because I'd like to understand the problem and why things are going a certain direction. Just so sometimes it helps me uncover other problems. We have with the code base or helps me find an issue with the change request, so this should be near. So let me update.

C

My suggestion here so it is in.

C

And I think that and I need to add a suggestion to add a new test.

C

C

So let me do that quickly, again, I'm showing to get them. So it's super explicit to what I'm thinking we should do and now it's up to the contributor to say: okay, I'm just gonna copy paste, the code or I'm just implementing something differently, because I can't think of as a something differently right and that's like the beautiful beautiful thing about core review.

A

Like you're, more collaborating.

C

To achieve one goal, instead of like hey, I'm telling you what to do and things like that, so it's always good to think of uh code review as pairing two works on something together instead of like I did the work now you review the work and I go back to my corner to.

A

C

Your comments, so I also enjoy thinking of code reviews like that.

C

So I'll add this to the review, and that is all for this one. So what I like to do is also run manual. Qa right and I like to document what I run as a manual qa um one as steven brought ground for myself. So if I come later either tomorrow or friday, when the community contributor submitted the changes, I was asking for or suggested I'll come back here copy paste. What steps I ran and it would be much quicker right, so no services to.

C

Find, for example, run.

C

B

Yes, disappear.

C

I'll just copy paste, the ci one. Are there any questions at the moment or like suggestions or feedback that you'd like to.

C

C

Okay, um I will not write the manual qa just because it's very very boring to see me type on screen so I'll do that later today, myself.

C

But let me submit the review and we just like let the other person.

C

C

But I reviewed it and you can take action on and.

C

So, uh to reiterate, just two comments, uh mostly tests, because the implementation itself looks like what we need and that's pretty much it for this one.

C

Not sure how we're doing on time.

C

C

I think we're right on time or do we have 10 minutes left christos.

A

uh It depends on whether you want to use. um We have 10 minutes left, but if you want to use them or not it's up to you, I also am not monitoring the guitar. There are no questions there. Neither on youtube.

C

I can do one more quick review on another one um perfect. So if we.

C

Look at the open office hour.

C

On we will see that we have this one from fabio, um so this is quite uh quite interesting. So uh previously we were testing some. uh um We wanted to attest about like long characters right and we were using a really long project id to make sure that is, it is being um uh concatenated properly.

C

Actually, yeah, it's being concatenated properly like this one right. But um if we look at this issue, this task is actually failing on 32-bit systems. Now we do support 32-bit, but we do not run our ci on 32-bit systems right and it's causing uh overflow uh into overflows, because this number is too big for 32-bit systems.

C

um Luckily, this was only in tests. It was never a production application, so it was still running. uh The debian folks are kind enough to run all their tasks on 22-bit and 64-bit and they flag.

C

This flag did for us, and this contribution is pretty much removing the test- and I agree with this, because this is such a nut case- that that is longer than 63 characters, uh project id as the uh id of the project inside of the database that is sent to us and it's a really really uh a really, really short chance, a small chance that sorry, that was a translation, english translation problem, a really short chance, uh slight chance that this will happen right.

C

So I think I'm fine removing this test because it does not really add any value. um So I think I'm just gonna merge it as this.

C

Let's double check the test cases to see if I'm missing something. So if we go to the smart.

C

C

C

And that is for line. What is.

C

C

And we have just basically testing the project, unique name right. um It's just making sure that we get the expected value, and that is unique for that specific project.

C

So and let's uh check so that the pipeline is failing. So, let's see if that is related to this change or not.

C

Now this one is a flaky pipeline, so I'm not sure if I can rebase no, I cannot replace it. uh So what I will ask the contributor is to rebase the smart request. So then we can just merge it. um I like to merge something in one that has a green pipeline. So if you're opening up a community contribution like this would be one of your first uh chocolate chat, chocolate checklist right make sure that the pipeline is coming before you send it over to a review.

C

um It's completely fine, if you don't uh sometimes it has to fail, and you do not understand why it's failing and that's why we're here to help so I'll, let fabio.

C

A

What is the number of this issue? Is it 27, 29 4.

C

It's 25.97 so I'll send it right actually great. Thank you so much from the chat as well cheers.

C

And that's so. These are small things that we have to go through like it's a small change.

C

um It's I wouldn't say it's a breaking change for production or anything like that, but this is still something we really really wanted to do, and I I really appreciate people just spending their own time doing this, so um I really enjoy reviewing these and uh I'm really really thankful for all the community work on this kind of stuff, because this is what keep what makes a project very healthy, uh teaming with talking contact dealing with these small educations, uh so we fix them up and uh make sure everything is working as expected.

C

So yep, that's it for me. Unless there are any questions.

A

Thank you. I have a question: um where can where can uh people find uh some issues, um easy issues to begin with or yeah.

C

Yeah, um I would say we follow the same methodology as the main gitlab project. So if I share my screen, I don't have a link handy to what the official guidelines say so christmas. You might be able to find that but like if you look at our issue tracker here and if you label by.

C

Accepting merchandise um I would say most of them are accepting merger burst, so you can look at this one and I'm pretty sure we also have.

A

Yeah, I was primarily talking about um the gitlab runner um project.

C

Yeah, this is on the kidlaprunner url, so uh we have our own issue tracker. uh Let me zoom in a bit more sorry about that under the kit lebron project- and these are all our issues for gridlock runner, um perfect. If you have any suggestions or ideas or things you want to improve or things that are bothering you um feel free to open an issue. um You can ping us directly my username as steve a.

A

C

Set so you can uh easily uh ping me and like I'll, either direct it to someone else or um we'll discuss it uh together on like if this issue is something that we want to solve or not, uh sometimes we get merged requests without an issue, and that is completely fine.

C

um We just need first, we need to discuss on the problem and if we want to solve the problem and how then, how we're going to solve it and if we're just opening up a merge request with that, that is somewhat already late uh in the process, but it is still completely fine with us that sometimes sometimes just showing the changes. What needs to be done to express what needs to be solved right so take it case by case basis,.

A

Thank you. Thank you. So much. um I'm gonna put that link on the hackathon page as well. uh With the gitlab issue, tracker and issues accepting we have a master issue where we track all the merge requests, and we also have some easy some labeled issues to begin with, for people so make sure that it's surfaced there as well.

A

There is a question from benson, I don't know yeah.

C

Yeah I'm reading right now so.

A

Let me just repeat it for the stream: uh how are these issues found uh test case? Removal seems like something that would remain and seems difficult for a user to motivate, as it would not use many uh cables.

C

Yeah so the first part, um the issue was found by the w community, actually so debian the debian project, uh which is a specific flavor of linux. Actually, packages gitlab runner for us and they run their tasks, their own tasks and package, gitlab runner in their own way, and they actually run their ci. So they run our tests on a 32-bit system and that's how that issue is found. They filed an issue on our issue tracker.

C

We saw that issue and then we said: okay, we'll solve this when we have time or we marked it as accepting, merge requests right now. The second part of your answer.

C

Removal seems like something that would remain and seem difficult for a user to motivate. um So I'm not sure I understand your question, but what I think you are saying is removing test cases might not be what users might want to do right, and that is completely fine. um I think there are multiple aspects of the project that can motivate you to work. Some people enjoy fixing the tests. Some people enjoy adding new features. Some people enjoy updating documentation. So I think we have everything we cover every aspect that you're interested in.

C

uh If I'm misunderstanding your question, please tell me, and I'm more than happy to answer.

A

Thank you all. Thank you all for participating. Thank you, steve hi darren as well. Thank you all for uh coming. uh We've already updated the website with the recording uh in case that people want to see it retrospectively, um steve any last words.

C

uh Just if you have any open merch, if you're planning to open any merch requests, uh feel free to bring me uh so we can try and get it merged and for the hackathon.

A

Thank you so much and again, if you have any any wider questions, you can find all of us on the getter channel on github, so jitters, dot, im, slash, gitlab, slash community, so feel free to join us there. You can find all the information information on their hackathon page and for the people who are going to join the next session. The release stage tutorial that is coming up in 10 minutes, see you soon for the rest. Happy hacking.

A