Add a Chart for GitLab CI Runner #608

twk3 · 2017-02-10T01:37:02Z

In order for this Chart to properly work, you need to specify the GitLab URL that you want to register the runner to. And the registration token, taken from the GitLab instance. (Either from the Shared Runners page for Admins, or any Project's CI/CD Pipelines settings)

k8s-ci-robot · 2017-02-10T01:37:03Z

Hi @twk3. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rothgar · 2017-02-10T01:49:41Z

Getting the token requires manual involvement from a user and kubectl execing into the gitlab pod. Is that correct?

Is there a way to export that token from the gitlab pod as a secret and consume the secret in the runner?

sameersbn · 2017-02-10T05:04:26Z

stable/gitlab-runner/templates/deployment.yaml

+          value: true
+        {{ end }}
+        - name: KUBERNETES_NAMESPACE
+          value: {{ default "" .Values.runners.namespace | quote }}


when not specified would it make sense to default to Release.Namespace instead?
ref: https://github.com/kubernetes/helm/blob/master/docs/charts.md#predefined-values

kwiesmueller · 2017-02-12T18:43:39Z

Hey, tested this with k8s on GCE and am getting errors with docker builds.
Seams like it is not mounting the docker.sock or the privileged flag is not working.

Is this a mistake on our side or something intended?

so0k · 2017-02-14T07:44:38Z

@rothgar - I tried to automate the token retrieval process here: https://github.com/honestbee/gitlab-infra#registering-multi-runner

…er deployment The value was being passed as a boolean when it should have been a string

twk3 · 2017-02-16T23:39:22Z

@sameersbn thanks, I added that in

@kwiesmueller there was a bug in how I passed the privileged env variable. I updated it, and it should now be fixed. kube clusters that allow privileged should work with dind if you flag privileged to true now.

kwiesmueller

That fixed it on my side as well. Thanks.

Dexyon · 2017-03-01T12:37:36Z

What can I do to help here? I just deployed the chart from a local clone and at first sight everything looks ok!

prydonius

This is looking great, I just have a few comments.

prydonius · 2017-03-16T15:56:37Z

stable/gitlab-runner/templates/NOTES.txt

@@ -0,0 +1,27 @@
+{{- if default "" .Values.gitlabUrl }}


Why do you use default here?

Copying what was used in the gitlab-ce chart. I've dropped them here now.

prydonius · 2017-03-16T15:57:47Z

stable/gitlab-runner/Chart.yaml

@@ -0,0 +1,15 @@
+name: gitlab-runner
+version: 0.1.0
+description: GitLab Runner


Can we improve the description and add an icon?

Updated with better description and added icon

kwiesmueller

Hey, there is a mismatch of the values in values.yaml and the template files.
The rest looks good, so far. Test instance is starting up.

kwiesmueller · 2017-03-23T17:36:05Z

stable/gitlab-runner/values.yaml

+## The GitLab Server URL (with protocol) that want to register the runner against
+## ref: https://docs.gitlab.com/runner/commands/README.html#gitlab-runner-register
+##
+# gitlabURL: http://gitlab.your-domain.com/


You got a not matching value here: gitlabURL in default values but gitlabUrl in all templates.
Just discovered this while testing.

Thanks, fixed

Fix values.yml reference to gitlabUrl, case issue Drop unnessesary defaulting in the Notes.txt Add better chart description, and include icon

twk3 · 2017-04-04T18:23:51Z

@prydonius I've updated the chart with the requested changes

twk3 · 2017-04-24T17:37:36Z

@prydonius are there any additional changes needed?

lsjostro · 2017-04-25T09:17:33Z

@twk3 would be nice if you could enable the metrics server for the runner.

rimusz · 2017-04-25T09:32:02Z

+1 for optional metrics

rimusz · 2017-05-10T10:05:27Z

@twk3 also readme should really include how to get runner Registration Token for selfhosted/hosted gitlab
I have intensively tested it with gitlab-ce, have not find any issues

kwiesmueller · 2017-05-10T12:45:04Z

Metrics would be nice to have, that's true...
We are using this chart in production for over 1 Month and got no issues. Everything is working great.

rimusz · 2017-05-10T14:16:21Z

@kwiesmueller can you share the image you use for kubernetes executors via runners.image:?

lsjostro · 2017-05-10T14:26:53Z

@rimusz @kwiesmueller this is my take on it:
https://gist.github.com/lsjostro/84c1ee027233382743c44722e285bbde

kwiesmueller · 2017-05-10T14:41:47Z

@rimusz the few builds currently running are using golang:1.8
The image set as default under runners.image is ubuntu:16.04 as it is not used anyways.

mgoodness · 2017-05-25T22:02:49Z

At the risk of offending everyone who's worked on this so far, I have to ask: is there value in adding & maintaining this chart when GitLab has their own, supported version?

https://gitlab.com/charts/charts.gitlab.io/tree/master/charts/gitlab-runner

#justasking

rothgar · 2017-05-25T22:15:14Z

Benefits of keeping this chart

Possible to have different implementations (maybe not a benefit)
Single repo to discover the chart from

Downsides

Probably not going to have as many features or be supported
Duplicating work
Need to add a separate repo

I'm in favor or getting rid of this one and using the chart provided by gitlab. Similarly I'm in favor of using containers from any project that provides their own containers (e.g. postgres) over community built/maintained containers.

It makes discovery harder but reduces duplicate work and gets features and fixes out faster. On the flip side, if someone is running kubernetes and wants to deploy gitlab they may not know about helm+charts. If gitlab documents "Here's how to deploy gitlab on kubernetes with this chart" it may let more people discover helm and the work happening here.

kwiesmueller · 2017-05-26T01:23:17Z

@mgoodness:
I didn't even know about the gitlab chart before I started working on my own one and then stumbled about this PR.
I would favor to keep it here, as it might help people looking for it in the official repos. Also I can not see any special features that would make a good reason to stay with the "official" gitlab one.

kwiesmueller · 2017-05-26T01:25:18Z

@rothgar:
I agree on the container part of your comment.
For various reasons we build all containers used in our clusters by ourselves as community managed containers tend to loose our trust by missing security updates etc.

twk3 · 2017-06-10T06:11:47Z

A similar conversation regarding the gitlab repo charts is taking place in this issue: #1138

viglesiasce · 2017-07-05T21:06:05Z

@twk3 I also agree that we should lean on the officially supported chart. I don't think the duplicate work is worth the maintenance burden.

We will be looking at how to delegate chart responsibility to other repositories soon and will update accordingly.

Do you mind if we close this out until then?

mgoodness · 2017-08-02T00:46:49Z

Closing in favor of the upstream chart. Feel free to re-open if/when it's decided otherwise.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2017

sameersbn reviewed Feb 10, 2017

View reviewed changes

lachie83 added the awaiting review label Feb 14, 2017

Fix privileged flag passed as environment variable in the gitlab-runn…

ed03f22

…er deployment The value was being passed as a boolean when it should have been a string

kwiesmueller approved these changes Feb 17, 2017

View reviewed changes

lachie83 modified the milestone: Milestone 4 Mar 9, 2017

prydonius suggested changes Mar 16, 2017

View reviewed changes

prydonius added changes needed and removed awaiting review labels Mar 16, 2017

lachie83 added the size/large label Mar 16, 2017

kwiesmueller suggested changes Mar 23, 2017

View reviewed changes

twk3 added 2 commits April 4, 2017 11:17

Apply review feedback for the gitlab-runner chart

1cd9770

Fix values.yml reference to gitlabUrl, case issue Drop unnessesary defaulting in the Notes.txt Add better chart description, and include icon

Update the gitlab-runner image to the latest stable version

752f675

kwiesmueller approved these changes Apr 4, 2017

View reviewed changes

mgoodness self-assigned this May 4, 2017

twk3 mentioned this pull request Jun 9, 2017

Gitlab Official Chart #1138

Closed

mgoodness closed this Aug 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Chart for GitLab CI Runner #608

Add a Chart for GitLab CI Runner #608

twk3 commented Feb 10, 2017

k8s-ci-robot commented Feb 10, 2017

rothgar commented Feb 10, 2017

sameersbn Feb 10, 2017

kwiesmueller commented Feb 12, 2017

so0k commented Feb 14, 2017

twk3 commented Feb 16, 2017

kwiesmueller left a comment

Dexyon commented Mar 1, 2017

prydonius left a comment

prydonius Mar 16, 2017

twk3 Apr 4, 2017

prydonius Mar 16, 2017

twk3 Apr 4, 2017

kwiesmueller left a comment •

edited

kwiesmueller Mar 23, 2017

twk3 Apr 4, 2017

twk3 commented Apr 4, 2017

twk3 commented Apr 24, 2017

lsjostro commented Apr 25, 2017

rimusz commented Apr 25, 2017 •

edited

rimusz commented May 10, 2017

kwiesmueller commented May 10, 2017

rimusz commented May 10, 2017

lsjostro commented May 10, 2017 •

edited

kwiesmueller commented May 10, 2017

mgoodness commented May 25, 2017 •

edited

rothgar commented May 25, 2017

kwiesmueller commented May 26, 2017

kwiesmueller commented May 26, 2017

twk3 commented Jun 10, 2017

viglesiasce commented Jul 5, 2017

mgoodness commented Aug 2, 2017

Add a Chart for GitLab CI Runner #608

Add a Chart for GitLab CI Runner #608

Conversation

twk3 commented Feb 10, 2017

k8s-ci-robot commented Feb 10, 2017

rothgar commented Feb 10, 2017

sameersbn Feb 10, 2017

Choose a reason for hiding this comment

kwiesmueller commented Feb 12, 2017

so0k commented Feb 14, 2017

twk3 commented Feb 16, 2017

kwiesmueller left a comment

Choose a reason for hiding this comment

Dexyon commented Mar 1, 2017

prydonius left a comment

Choose a reason for hiding this comment

prydonius Mar 16, 2017

Choose a reason for hiding this comment

twk3 Apr 4, 2017

Choose a reason for hiding this comment

prydonius Mar 16, 2017

Choose a reason for hiding this comment

twk3 Apr 4, 2017

Choose a reason for hiding this comment

kwiesmueller left a comment • edited

Choose a reason for hiding this comment

kwiesmueller Mar 23, 2017

Choose a reason for hiding this comment

twk3 Apr 4, 2017

Choose a reason for hiding this comment

twk3 commented Apr 4, 2017

twk3 commented Apr 24, 2017

lsjostro commented Apr 25, 2017

rimusz commented Apr 25, 2017 • edited

rimusz commented May 10, 2017

kwiesmueller commented May 10, 2017

rimusz commented May 10, 2017

lsjostro commented May 10, 2017 • edited

kwiesmueller commented May 10, 2017

mgoodness commented May 25, 2017 • edited

rothgar commented May 25, 2017

kwiesmueller commented May 26, 2017

kwiesmueller commented May 26, 2017

twk3 commented Jun 10, 2017

viglesiasce commented Jul 5, 2017

mgoodness commented Aug 2, 2017

kwiesmueller left a comment •

edited

rimusz commented Apr 25, 2017 •

edited

lsjostro commented May 10, 2017 •

edited

mgoodness commented May 25, 2017 •

edited