GitLab GitLab Runner Open Office Hours, 2 Sep 2020

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: September2020 Runner office hour

Description

Reviewing two community contributions for the GitLab Runner project.

- Documentation for configuring private registries (https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/2131)
- Add Docker ulimit support (https://gitlab.com/gitlab-org/gitlab-runner/-/merge_requests/1957)

A

Okay, welcome to uh this month's uh runner office hour. It's this is becoming a tradition uh happening during the hackathon. So without further ado, steve I'll turn things over to you, and then we got some, mrs, that uh that are your candidates for reviews, so we'll just get things started.

B

Yeah hi everyone. um So unless there are questions or.

C

Any things that you want to discuss during now.

B

um I can start reviewing some community contributions that I put up this morning that I think are a good candidate.

B

Screen so I thought we could start with a fairly very decent one, which is a documentation change.

B

The documentation changes are fairly easy to look at and merge so.

C

um I'm just gonna look at this.

B

Because it's been hanging for a while, so it.

C

Should yeah, we should be able to try to merge it. So this one.

B

D

B

Using private registry for.

C

The kubernetes executor.

C

B

At the comments here uh like uh documentation changes, you should go through um both two kinds of reviews, so I review from the technical phones, people like me and their v from technical writers, which are the people who own the documentation.

C

B

There are a lot better writers than I am for sure. So I see this one was reviewed both by marcel and susan, and they approved it. So from a grammar perspective, documentation perspective, it seems to be all good. I just need to verify that's good from a technical.

C

B

So this is one that's adding.

C

I'm looking to put in these dogs and then it's just fine.

D

B

And this is about I'm guessing. This is talking about our health chart. Actually.

C

C

So I just need to check our hamstring with actual supports, just because I'm not.

C

B

B

Yes, okay, so basically what this merge quest is just documenting the image pull speakers that we already have here.

C

That seems to be fine. One concern I have about.

B

C

B

C

We added support for docker autoconfig, so.

B

So perform with the kubernetes executor. The only way you could use private images um with gitlab runner is using these image for secrets.

B

B

We started supporting docker odd config, even for the kubernetes executor. So if you specify this as a variable and your ci job, you can use that authentication to pull in the private images, so I'm gonna merge it as a test, but what I will do after this is open up.

B

Another follow-up merge request to specify we have more options than this and the reason I'm doing this is so we get this community contribution in, but then we iterate on the documentation to make it even better, because this is good, but it can be improved later on and we can expose.

C

It a bit more so um what I'm going to do is leave a comment here.

D

And I'm gonna try and find the issue.

D

C

Let me try and find these.

B

D

D

C

C

C

So I'm gonna add this comment and with good luck we can resolve the strategy issue so we'll do.

C

C

C

And then we can arrange this one right.

B

Here so this, like.

C

A

B

uh Documentation change, but I didn't want like I don't think we ever showed.

C

What the documentation measures looks like and what kind of code goes through, so I wanted to share that really quick.

A

Well, I appreciate yeah you're doing this uh for a simple doing, a simple merge, uh because I think it looks like it's been sitting around for a while, but it's a good idea to get this merge and improve it later on.

C

A

Yeah go ahead. Sorry yeah.

C

B

No, uh it's always kind of a case by case basis, and it's something I already struggled with personally like if we should merge it as it is and then improve it later on or if we should ask the community contributor to fix it. um I still don't know what the right answer is, so it's kind of going on a gut feeling at this point right so far, it's never been like a.

A

Bad experience.

B

And I think we've actually had returning contributors just because we're merged there and merged with us, and we improved it later on so.

C

So far, it seems to be working, fine.

A

Yeah, no, I think it may have been reasonable to suggest uh adding additional like info there uh if this was just open, but I think this works well, and the other thing I just wanted to mention was that if it's like a truly simple change like uh like a type of fix or grammar fix, then I mean usually technical writers will just merge them, as is without even checking with um engineers, in this case, on the runner team.

A

But uh but if it involves like a validation of a technical feature or something, uh then uh engineers will get ping. But if it's a simple one like you'll, typically see them, merge like right away.

B

C

B

Team is very, very responsive.

C

When it comes to merger quests, so um yeah.

B

As long as we're they're not blocked by us they're, usually.

C

B

One is added to the merge train that will be merged as soon as this pipeline passes, so we can.

C

C

Next one, which is adding a new.

B

Setting for supporting docker? U limits! So uh we have an issue around this where uh the docker executor. We already have limits for the cpu and memory, but we don't have.

C

Support for units now to occur, you just do quick search and property.

C

B

Basically, it's a way for us to limit certain priorities and things like that, and we can see this as well. If we do docker run for.

C

C

You can actually see this.

B

C

By default, it's empty.

B

And so far we always send empty to the docker.

C

Api, so every.

B

So we have an issue attached to this request and I like that is always the like the best scenario. We have an issue, that's already discussed and we've already have.

C

B

C

Of what we need to implement.

B

And why there is the need for it, so that would be just a matter of reviewing this they're like usually, if we just get a merge request out of the blue in the in the sense that we never really discussed or anything like that. First, we have to go through the discussion here. Like is this what we want, why we want this, uh how it's gonna, help our customers and so on and so forth? But since this is pretty clear what needs to be done, uh we don't have to discuss that.

C

In the merge quest so.

B

uh You limit here it's adding a new configuration file, which is a map.

C

C

C

Is so just in the translation? Stop, I'm just gonna! Look at this face.

C

Let's get into here. Let's check mutation here we're returning that and then let's attach it to this one. So let me check out the.

C

C

C

D

C

So one thing that I'm already saying is we added stuff to the config.config.goldfile, but I do not see any config underscore test changes. So probably this part is not really well tested on a union club, but I suspect it's tested on an integration.

C

uh Ideally, we should have both on both levels. Just so we make sure that that's not getting, uh but let's see, let's open up this.

C

C

C

D

C

C

C

C

Okay, so we're calling this function directly here, but where this test exists in the docker execution test. So one thing that one suggestion that I think.

C

C

To move this one into the config file instead of the docker executable and then the executor one will be a proper.

C

C

C

Soft and applied values so.

C

Right came in with the map.

D

D

C

Okay, so the waivers seem to be translating this is its name and then so.

C

This is a string.

C

C

You should also do this because here, if we expect to know that we are checking that we get our output here, when we don't expect an error very much. But there is no.

C

D

C

C

So one other thing that comes to mind. It seems like we're. I haven't looked at the function yet, but it seems like we're parsing strings and then translating these strings, which, from past experiences it's never been as straightforward as possible and not very um if we want to change the person, but we would have to do a lot of breaking changes and things like that, so I'm gonna see how talker specifies it. If it's the same will stick to this, but the docker specifies it in a different way. We might have to.

D

D

D

C

So here it's doing the same.

C

C

Okay, so the only thing I'm afraid of this is.

C

The person of it right, I think, if we have a different structure so of using stream, um I think for each one. So for.

C

C

I'm not even sure one.

C

C

B

I'm just trying to.

C

Understand how the user is going to configure this.

C

C

C

C

C

C

Much depends on our.

C

C

Last second,.

C

C

C

You can see that the room is.

C

So what I'm thinking.

C

C

We'll have something like.

C

C

C

C

So it's a bit more clear: um docker ends up introducing either a new field or new meanings. With these you can easily have a key here to whatever it goes to, for example, and we don't have to mess with our string parts whatsoever.

B

Before we used to do it like.

C

String parsing, but like things change out of our control and like we've, always had headaches with this uh migrating to a new structure like, for example, one id one case was services right before we used to define a service from the conflict autonomous file with just the name of the service. But then we wanted to specify isis and so on and so forth. More configuration with the service before it used to be just a string. But now it's a complete structure so.

C

That resulted into a bunch of changes, I'm not a hundred percent sure if this is the right choice or not. So what I'm going to do is.

C

Just add a suggestion and see what the contributor.

C

C

D

C

Those are my two suggestions here.

C

C

Since this is a map of the string, one um one thing that we also have to keep in mind, these are not acceptable by your environment variables when we specify f colon and the name. That means that when I.

C

C

But in this case this complex structure, which is a map- that's not something that we can choose from.

C

See if howard decide regarding this and then we can discuss uh if we should remove this one or not,.

C

Now the last thing to test to check is.

C

C

C

Most of it is covered apart.

C

C

C

B

C

If we can get the coverage fairly.

C

C

C

C

So yeah, let's test the software this one, so we just need another.

C

C

Like this, we make sure that your make sure it doesn't each side we're.

D

D

C

C

C

Splitting the string quieter.

C

C

C

C

C

Okay, this is that.

D

C

Okay, there we go it's just I have the old coverage showing that's why it doesn't show that this part. So here we added two new test cases.

C

C

I was asking for and like if the user does not understand my words or um it's a bit more actionable for the contributor to see what I was asking for and what be.

C

So for now that seems to be it um we'll discuss. This part asynchronous will move the test, and I ask the contributor to add a bit more tests and then we'll see how that goes.

C

I like to give like a wrap-up.

D

D

C

C

C

We have 10 minutes left, so I don't think I will pick up any merchandise because that's not enough! Thank you. If you remember this, uh but if there's any questions whatsoever or anything, we want to discuss today more than happy to answer.

A

Cool, no, I mean thanks for doing this. It's the second one definitely was more involved, but uh no glad we got to review a documentation as well. Like you said, I don't think we've done that before so yeah maria- I don't know if you have any questions or anything else related to runner in general,.

A

So yeah, hopefully you found it helpful. uh I mean if you missed something uh they'll be recording on this uh on the youtube channel as well, um but yeah. If you happen to open a mrn runner feel free to ping steve directly, um you don't have to wait for to get triage so.

B

I'll be more than happy to help and if.

C

I'm at the capacity like I don't have time to recruit in my timely manner. um We have other four team members. So I'll pick, one of those and they'll help you out as well so yeah.

D

Okay, thank you very much.

A

All right all right, thanks, steve, have a good evening right, cheers bye,.

D