[libvirt-jenkins-ci PATCH v2 0/6] Introduce the new 'gitlab' flavor
since v1: - .bash_logout is removed from the user home environment (causes builds to fail on Debian) - gitlab-runner is now downloaded to and executed from user's bin/ directory instead of using /usr/local/bin - both RC and systemd service files now define the user explicitly that the init system changes to when spawning the gitlab-runner service - the gitlab flavour tasks file now relies on shell's 'creates' module argument so that the agent registration is idempotent rather than touching the dirs/files explicitly - both gitlab URL and runner token are now files editable under ~/.config/lcitool Both the config.toml improvement in terms of replacing the ~/.config/lcitool files as discussed during the v1 review process as well as cloudinit usage is planned for a separate respective series, so for now, we'll have to live with 2 new config files and cloud images will need to be built manually. Erik Skultety (6): guests: users: Discard the .bash_logout skeleton file guests: users: Create a bin/ directory in the flavor user's home guests: templates: Introduce a gitlab-runner systemd service template guests: templates: Introduce a gitlab-runner RC init service template guests: Introduce the new 'gitlab' flavor guests: lcitool: Enable the new 'gitlab' flavor in the lcitool script guests/lcitool | 50 ++++++++++++++++-- guests/playbooks/update/main.yml | 5 ++ guests/playbooks/update/tasks/gitlab.yml | 52 +++++++++++++++++++ guests/playbooks/update/tasks/users.yml | 10 +++- .../update/templates/gitlab-runner.j2 | 31 +++++++++++ .../update/templates/gitlab-runner.service.j2 | 14 +++++ 6 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 guests/playbooks/update/tasks/gitlab.yml create mode 100644 guests/playbooks/update/templates/gitlab-runner.j2 create mode 100644 guests/playbooks/update/templates/gitlab-runner.service.j2 -- 2.25.1
As part of the users tasks, we remove the .profile file after we create our own .bash_profile. As part of that effort, let's remove the .bash_logout skeleton file as well, the reason being gitlab's runner agent which has a hard time initializing a build environment on Debian where the system populates the .bash_logout with console cleaning code which messes up the runner and causes the build to fail instantly [1]. Since Debian is the only distro actually populating the file and the machines are assumed to be accessed over SSH, we can safely drop it. [1] https://gitlab.com/gitlab-org/gitlab-runner/issues/4449 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/playbooks/update/tasks/users.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/guests/playbooks/update/tasks/users.yml b/guests/playbooks/update/tasks/users.yml index 30e5fca..a07349f 100644 --- a/guests/playbooks/update/tasks/users.yml +++ b/guests/playbooks/update/tasks/users.yml @@ -63,9 +63,10 @@ - bash_profile - bashrc -- name: '{{ flavor }}: Remove existing shell profile' +- name: '{{ flavor }}: Remove unnecessary home skeleton files' file: path: /home/{{ flavor }}/.{{ item }} state: absent with_items: - profile + - bash_logout -- 2.25.1
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
As part of the users tasks, we remove the .profile file after we create our own .bash_profile. As part of that effort, let's remove the .bash_logout skeleton file as well, the reason being gitlab's runner agent which has a hard time initializing a build environment on Debian where the system populates the .bash_logout with console cleaning code which messes up the runner and causes the build to fail instantly [1]. Since Debian is the only distro actually populating the file and the machines are assumed to be accessed over SSH, we can safely drop it.
[1] https://gitlab.com/gitlab-org/gitlab-runner/issues/4449
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/playbooks/update/tasks/users.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
We're creating a dedicated user to run the gitlab agent, so why not store the agent within the user profile and execute it from there. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/playbooks/update/tasks/users.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/guests/playbooks/update/tasks/users.yml b/guests/playbooks/update/tasks/users.yml index a07349f..4b09416 100644 --- a/guests/playbooks/update/tasks/users.yml +++ b/guests/playbooks/update/tasks/users.yml @@ -70,3 +70,10 @@ with_items: - profile - bash_logout + +- name: '{{ flavor }}: Create /home/{{ flavor }}/bin directory' + file: + path: /home/{{ flavor }}/bin + state: directory + owner: '{{ flavor }}' + group: '{{ flavor }}' -- 2.25.1
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
We're creating a dedicated user to run the gitlab agent, so why not store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector. ie malicious code running as the gitlab account can replace the gitlab agent binary in its $HOME. Shouldn't the binary be in /usr/local/bin and owned by root so it is completely separated ?
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/playbooks/update/tasks/users.yml | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/guests/playbooks/update/tasks/users.yml b/guests/playbooks/update/tasks/users.yml index a07349f..4b09416 100644 --- a/guests/playbooks/update/tasks/users.yml +++ b/guests/playbooks/update/tasks/users.yml @@ -70,3 +70,10 @@ with_items: - profile - bash_logout + +- name: '{{ flavor }}: Create /home/{{ flavor }}/bin directory' + file: + path: /home/{{ flavor }}/bin + state: directory + owner: '{{ flavor }}' + group: '{{ flavor }}' -- 2.25.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
We're creating a dedicated user to run the gitlab agent, so why not store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector. ie malicious code running as the gitlab account can replace the gitlab agent binary in its $HOME.
Shouldn't the binary be in /usr/local/bin and owned by root so it is completely separated ?
That's what I've done in v1 (though not because of the possible attack vector you mention), but it was suggested to move it to user's $HOME [1]. [1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html I'll change it to the original version on my local branch.
On Tue, Apr 07, 2020 at 01:45:46PM +0200, Erik Skultety wrote:
On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
We're creating a dedicated user to run the gitlab agent, so why not store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector. ie malicious code running as the gitlab account can replace the gitlab agent binary in its $HOME.
Shouldn't the binary be in /usr/local/bin and owned by root so it is completely separated ?
That's what I've done in v1 (though not because of the possible attack vector you mention), but it was suggested to move it to user's $HOME [1]. [1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html
I'll change it to the original version on my local branch.
Hmm, for that matter, we shouldn't store the config file in the /home/gitlab/.gitlab-runner directory either. Essentially we should try to assume anything in $HOME is subjec to arbitrary deletion in order to restore a clean state, so we shouldn't try to keep important files there. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Apr 07, 2020 at 12:48:34PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:45:46PM +0200, Erik Skultety wrote:
On Tue, Apr 07, 2020 at 12:37:01PM +0100, Daniel P. Berrangé wrote:
On Tue, Apr 07, 2020 at 01:31:17PM +0200, Erik Skultety wrote:
We're creating a dedicated user to run the gitlab agent, so why not store the agent within the user profile and execute it from there.
I'm wary of this as it seems like it can create a exploit vector. ie malicious code running as the gitlab account can replace the gitlab agent binary in its $HOME.
Shouldn't the binary be in /usr/local/bin and owned by root so it is completely separated ?
That's what I've done in v1 (though not because of the possible attack vector you mention), but it was suggested to move it to user's $HOME [1]. [1] https://www.redhat.com/archives/libvir-list/2020-March/msg01424.html
I'll change it to the original version on my local branch.
Hmm, for that matter, we shouldn't store the config file in the /home/gitlab/.gitlab-runner directory either.
Yes, I'll make sure the config is under the default system location in /etc/gitlab-runner/ with read permissions for the gitlab user. Thanks, -- Erik Skultety
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .../update/templates/gitlab-runner.service.j2 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 guests/playbooks/update/templates/gitlab-runner.service.j2 diff --git a/guests/playbooks/update/templates/gitlab-runner.service.j2 b/guests/playbooks/update/templates/gitlab-runner.service.j2 new file mode 100644 index 0000000..5d494b2 --- /dev/null +++ b/guests/playbooks/update/templates/gitlab-runner.service.j2 @@ -0,0 +1,14 @@ +[Unit] +Description=GitLab Runner +After=network.target +ConditionFileIsExecutable=/home/gitlab/bin/gitlab-runner + +[Service] +User=gitlab +WorkingDirectory=/home/gitlab +ExecStart=/home/gitlab/bin/gitlab-runner run --config /home/gitlab/.gitlab-runner/config.toml +Restart=always + +[Install] +WantedBy=multi-user.target + -- 2.25.1
Based on https://docs.gitlab.com/runner/install/freebsd.html#installing-gitlab-runner Signed-off-by: Erik Skultety <eskultet@redhat.com> --- .../update/templates/gitlab-runner.j2 | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 guests/playbooks/update/templates/gitlab-runner.j2 diff --git a/guests/playbooks/update/templates/gitlab-runner.j2 b/guests/playbooks/update/templates/gitlab-runner.j2 new file mode 100644 index 0000000..bfd722b --- /dev/null +++ b/guests/playbooks/update/templates/gitlab-runner.j2 @@ -0,0 +1,31 @@ +#!/bin/sh +# PROVIDE: gitlab_runner +# REQUIRE: DAEMON NETWORKING +# BEFORE: +# KEYWORD: + +. /etc/rc.subr + +name="gitlab_runner" +rcvar="gitlab_runner_enable" + +user="gitlab" +user_home="/home/gitlab" +command="/home/gitlab/bin/gitlab-runner" +command_args="run --config ${user_home}/.gitlab-runner/config.toml" +pidfile="/var/run/${name}.pid" + +start_cmd="gitlab_runner_start" + +gitlab_runner_start() +{ + export USER=${user} + export HOME=${user_home} + if checkyesno ${rcvar}; then + cd ${user_home} + /usr/sbin/daemon -u ${user} -p ${pidfile} ${command} ${command_args} > /var/log/gitlab-runner.log 2>&1 + fi +} + +load_rc_config $name +run_rc_command $1 -- 2.25.1
With the recent efforts in upstream libvirt to centralize our CI on gitlab, let's add a new gitlab-specific flavor along with related playbook tasks. This flavour revolves around installing and configuring the gitlab-runner agent binary which requires the per-project registration token to be specified in order for the runner to be successfully registered with the gitlab server. Note that as part of the registration process each runner acquires a new unique access token. This means that we must ensure that the registration is run only on the first update, otherwise a new runner with a new access token is registered with the gitlab project. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/playbooks/update/main.yml | 5 +++ guests/playbooks/update/tasks/gitlab.yml | 52 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 guests/playbooks/update/tasks/gitlab.yml diff --git a/guests/playbooks/update/main.yml b/guests/playbooks/update/main.yml index a5a4de8..371e53d 100644 --- a/guests/playbooks/update/main.yml +++ b/guests/playbooks/update/main.yml @@ -58,3 +58,8 @@ - include: '{{ playbook_base }}/tasks/jenkins.yml' when: - flavor == 'jenkins' + + # Install the Gitlab runner agent + - include: '{{ playbook_base }}/tasks/gitlab.yml' + when: + - flavor == 'gitlab' diff --git a/guests/playbooks/update/tasks/gitlab.yml b/guests/playbooks/update/tasks/gitlab.yml new file mode 100644 index 0000000..b8f731d --- /dev/null +++ b/guests/playbooks/update/tasks/gitlab.yml @@ -0,0 +1,52 @@ +--- +- name: Define gitlab-related facts + set_fact: + gitlab_url: '{{ lookup("file", gitlab_url_file) }}' + gitlab_runner_secret: '{{ lookup("file", gitlab_runner_token_file) }}' + gitlab_runner_download_url: https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-{{ ansible_system|lower }}-amd64 + gitlab_runner_config_path: '/home/gitlab/.gitlab-runner/config.toml' + +- name: Download gitlab-runner agent + get_url: + url: '{{ gitlab_runner_download_url }}' + dest: /home/gitlab/bin/gitlab-runner + owner: gitlab + group: gitlab + mode: '0775' + force: yes + +- name: Register the gitlab-runner agent + become: true + become_user: gitlab + shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{ gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{ gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}' + args: + creates: '{{ gitlab_runner_config_path }}' + +- block: + - name: Install the gitlab-runner service unit + template: + src: '{{ playbook_base }}/templates/gitlab-runner.service.j2' + dest: /etc/systemd/system/gitlab-runner.service + + - name: Enable the gitlab-runner service + systemd: + name: gitlab-runner + state: started + enabled: yes + daemon_reload: yes + when: ansible_service_mgr == 'systemd' + +- block: + - name: Install the gitlab_runner rc service script + template: + src: '{{ playbook_base }}/templates/gitlab-runner.j2' + dest: '/usr/local/etc/rc.d/gitlab_runner' + mode: '0755' + + - name: Enable the gitlab-runner rc service + service: + name: gitlab_runner + state: started + enabled: yes + when: ansible_service_mgr != 'systemd' + -- 2.25.1
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
+- name: Register the gitlab-runner agent + become: true + become_user: gitlab + shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{ gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{ gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}'
You didn't answer in the other thread, so I'll ask again here: is the idea that we're going to use only the FreeBSD runners to supplement the shared runners for the existing unit tests, and all Linux runners are only going to be used for integration tests later on, hence why we need to use the shell executor rather than the docker executor? Additional nit: instead of using {{ inventory_hostname }} as tag, we can have a nicer tag by using {{ os_name|lower }}-{{ os_version }}. It would also be a good idea to quote all command arguments. The rest looks good, but given the potential security issue raised by Dan I'll wait for v3 before handing out actual ACKs :) -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
+- name: Register the gitlab-runner agent + become: true + become_user: gitlab + shell: '/home/gitlab/bin/gitlab-runner register --non-interactive --config {{ gitlab_runner_config_path }} --registration-token {{ gitlab_runner_secret }} --url {{ gitlab_url }} --executor shell --tag-list {{ inventory_hostname }}'
You didn't answer in the other thread, so I'll ask again here: is the idea that we're going to use only the FreeBSD runners to supplement the shared runners for the existing unit tests, and all Linux runners are only going to be used for integration tests later on, hence why we need to use the shell executor rather than the docker executor?
Why not both? We can always extend the capacity with VM builders, although it's true that functional tests was what I had in mind originally (+the FreeBSD exception like you already mentioned). Either way, I don't understand why we should force usage of the docker executor for the VMs which we can use for builds. The way I'm looking at the setup is: container setup vs VM setup, with both being consistent in their own respective category, IOW, why should the setup of the VM in terms of the gitlab-runner be different when running functional tests vs builds? So, I'd like to stay with the shell executor on VMs in all cases and not fragment the setup even further. Furthermore, with the OpenShift infra we got, I see very little to no value in using the VMs to extend our build capacity.
Additional nit: instead of using {{ inventory_hostname }} as tag, we can have a nicer tag by using {{ os_name|lower }}-{{ os_version }}.
Sure, I don't mind that change.
It would also be a good idea to quote all command arguments.
Will do. -- Erik Skultety
On Wed, 2020-04-08 at 12:28 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
You didn't answer in the other thread, so I'll ask again here: is the idea that we're going to use only the FreeBSD runners to supplement the shared runners for the existing unit tests, and all Linux runners are only going to be used for integration tests later on, hence why we need to use the shell executor rather than the docker executor?
Why not both? We can always extend the capacity with VM builders, although it's true that functional tests was what I had in mind originally (+the FreeBSD exception like you already mentioned). Either way, I don't understand why we should force usage of the docker executor for the VMs which we can use for builds. The way I'm looking at the setup is: container setup vs VM setup, with both being consistent in their own respective category, IOW, why should the setup of the VM in terms of the gitlab-runner be different when running functional tests vs builds? So, I'd like to stay with the shell executor on VMs in all cases and not fragment the setup even further.
Because all the builds that currently exist are defined in terms of containers, so when you have something like x86-fedora-31: script: ... image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest you cannot just run the same job on a worker that is configured to use the shell executor. I guess you could drop the image element and replace it with tags: - fedora-31 but then you'd either have to duplicate the job definition, or to only have the new one which then would not work for forks and merge requests, so that makes it less interesting.
Furthermore, with the OpenShift infra we got, I see very little to no value in using the VMs to extend our build capacity.
I don't understand what you're trying to say here at all, sorry. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 08, 2020 at 03:21:00PM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-08 at 12:28 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 12:05:11PM +0200, Andrea Bolognani wrote:
You didn't answer in the other thread, so I'll ask again here: is the idea that we're going to use only the FreeBSD runners to supplement the shared runners for the existing unit tests, and all Linux runners are only going to be used for integration tests later on, hence why we need to use the shell executor rather than the docker executor?
Why not both? We can always extend the capacity with VM builders, although it's true that functional tests was what I had in mind originally (+the FreeBSD exception like you already mentioned). Either way, I don't understand why we should force usage of the docker executor for the VMs which we can use for builds. The way I'm looking at the setup is: container setup vs VM setup, with both being consistent in their own respective category, IOW, why should the setup of the VM in terms of the gitlab-runner be different when running functional tests vs builds? So, I'd like to stay with the shell executor on VMs in all cases and not fragment the setup even further.
Because all the builds that currently exist are defined in terms of containers, so when you have something like
x86-fedora-31: script: ... image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
you cannot just run the same job on a worker that is configured to use the shell executor.
I guess you could drop the image element and replace it with
tags: - fedora-31
^This is what I had in mind
but then you'd either have to duplicate the job definition, or to only have the new one which then would not work for forks and merge requests, so that makes it less interesting.
We'll have to duplicate it for FreeBSD anyway, so I don't understand why should do it differently for other VM distros.
Furthermore, with the OpenShift infra we got, I see very little to no value in using the VMs to extend our build capacity.
I don't understand what you're trying to say here at all, sorry.
What I meant is that I don't see much value to run the builds in VMs since we have a bunch of images ready where we can already execute the builds so it's basically only about having resources to spawn the containers and that's where OpenShift comes in. I understand you reminding me that private runners cannot be used to run on merge requests (and forks for obvious reasons), but the same applies to VMs we're discussing in this thread. So, I wouldn't focus primarily on running the builds there is what I'm saying. -- Erik Skultety
On Wed, 2020-04-08 at 15:56 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 03:21:00PM +0200, Andrea Bolognani wrote:
I guess you could drop the image element and replace it with
tags: - fedora-31
^This is what I had in mind
but then you'd either have to duplicate the job definition, or to only have the new one which then would not work for forks and merge requests, so that makes it less interesting.
We'll have to duplicate it for FreeBSD anyway, so I don't understand why should do it differently for other VM distros.
We will not duplicate it, because there is no existing container based build for FreeBSD: the FreeBSD builds can, by definition, only happen inside VMs.
I don't understand what you're trying to say here at all, sorry.
What I meant is that I don't see much value to run the builds in VMs since we have a bunch of images ready where we can already execute the builds
Totally agree with you up until this point: this is exactly what I was trying to convey.
so it's basically only about having resources to spawn the containers and that's where OpenShift comes in.
I still don't understand how OpenShift is part of the picture. I have probably either missed or forgotten something O:-)
I understand you reminding me that private runners cannot be used to run on merge requests (and forks for obvious reasons), but the same applies to VMs we're discussing in this thread. So, I wouldn't focus primarily on running the builds there is what I'm saying.
I think we're kind of talking past each other at this point :) To reiterate/clarify: I'm perfectly okay with using Linux VMs for integration testing only and leaving builds to shared runner and FreeBSD VMs. I don't think we should use Linux VMs for builds unless we use the docker executor for them, but then again I don't think we really need to use Linux VMs for builds in the first place. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 689a8cf..d9b2372 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -175,7 +175,7 @@ class Config: ) ) - if flavor not in ["test", "jenkins"]: + if flavor not in ["test", "jenkins", "gitlab"]: raise Exception("Invalid flavor '{}'".format(flavor)) return flavor @@ -185,7 +185,7 @@ class Config: # The vault password is only needed for the jenkins flavor, but in # that case we want to make sure there's *something* in there - if self.get_flavor() != "test": + if self.get_flavor() == "jenkins": vault_pass_file = self._get_config_file("vault-password") try: @@ -217,6 +217,38 @@ class Config: return root_pass_file + def get_gitlab_runner_token_file(self): + gitlab_runner_token_file = self._get_config_file("gitlab-runner-token") + + try: + with open(gitlab_runner_token_file, "r") as infile: + if not infile.readline().strip(): + raise ValueError + except Exception as ex: + raise Exception( + "Missing or invalid gitlab runner token file ({}): {}".format( + gitlab_runner_token_file, ex + ) + ) + + return gitlab_runner_token_file + + def get_gitlab_url_file(self): + gitlab_url_file = self._get_config_file("gitlab-url") + + try: + with open(gitlab_url_file, "r") as infile: + if not infile.readline().strip(): + raise ValueError + except Exception as ex: + raise Exception( + "Missing or invalid gitlab url file ({}): {}".format( + gitlab_url_file, ex + ) + ) + + return gitlab_url_file + class Inventory: @@ -449,6 +481,8 @@ class Application: flavor = self._config.get_flavor() vault_pass_file = self._config.get_vault_password_file() root_pass_file = self._config.get_root_password_file() + gitlab_url_file = self._config.get_gitlab_url_file() + gitlab_runner_token_file = self._config.get_gitlab_runner_token_file() ansible_hosts = ",".join(self._inventory.expand_pattern(hosts)) selected_projects = self._projects.expand_pattern(projects) @@ -469,7 +503,7 @@ class Application: playbook_base = os.path.join(base, "playbooks", playbook) playbook_path = os.path.join(playbook_base, "main.yml") - extra_vars = json.dumps({ + extra_vars_d = { "base": base, "playbook_base": playbook_base, "root_password_file": root_pass_file, @@ -477,7 +511,13 @@ class Application: "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, - }) + } + + if flavor == "gitlab": + extra_vars_d.update([ + ("gitlab_url_file", gitlab_url_file), + ("gitlab_runner_token_file", gitlab_runner_token_file), + ]) ansible_playbook = distutils.spawn.find_executable("ansible-playbook") if ansible_playbook is None: @@ -486,7 +526,7 @@ class Application: cmd = [ ansible_playbook, "--limit", ansible_hosts, - "--extra-vars", extra_vars, + "--extra-vars", json.dumps(extra_vars_d), ] # Provide the vault password if available -- 2.25.1
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
# The vault password is only needed for the jenkins flavor, but in # that case we want to make sure there's *something* in there - if self.get_flavor() != "test": + if self.get_flavor() == "jenkins": vault_pass_file = self._get_config_file("vault-password")
You need something similar to this in your new functions, otherwise trying even if you've configured lcitool to use the test flavor you'll still get $ ./lcitool update all all ./lcitool: Missing or invalid gitlab url file ...
+ def get_gitlab_runner_token_file(self): + gitlab_runner_token_file = self._get_config_file("gitlab-runner-token") + + try: + with open(gitlab_runner_token_file, "r") as infile: + if not infile.readline().strip(): + raise ValueError + except Exception as ex: + raise Exception( + "Missing or invalid gitlab runner token file ({}): {}".format(
Please capitalize GitLab properly in user-visible strings.
- extra_vars = json.dumps({ + extra_vars_d = {
You don't really need to change the name of the variable.
+ if flavor == "gitlab": + extra_vars_d.update([ + ("gitlab_url_file", gitlab_url_file), + ("gitlab_runner_token_file", gitlab_runner_token_file), + ])
You can use a dict as argument to update(), which is more natural than a list of tuples. But in reality I think you can skip the check entirely and add both keys to extra_vars unconditionally: after you've fixed the issue I pointed out above, they will either be set (gitlab flavor) or None (any other flavor), and even in the latter case it's fine to pass them to Ansible as they will simply not be used. Everything else looks good. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Apr 08, 2020 at 11:49:45AM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
# The vault password is only needed for the jenkins flavor, but in # that case we want to make sure there's *something* in there - if self.get_flavor() != "test": + if self.get_flavor() == "jenkins": vault_pass_file = self._get_config_file("vault-password")
You need something similar to this in your new functions, otherwise trying even if you've configured lcitool to use the test flavor you'll still get
$ ./lcitool update all all ./lcitool: Missing or invalid gitlab url file ...
Doh! Clearly I didn't test regressions :(
+ def get_gitlab_runner_token_file(self): + gitlab_runner_token_file = self._get_config_file("gitlab-runner-token") + + try: + with open(gitlab_runner_token_file, "r") as infile: + if not infile.readline().strip(): + raise ValueError + except Exception as ex: + raise Exception( + "Missing or invalid gitlab runner token file ({}): {}".format(
Please capitalize GitLab properly in user-visible strings.
- extra_vars = json.dumps({ + extra_vars_d = {
You don't really need to change the name of the variable.
Right, although there are certain good practices like adding _l to lists and _d to dicts, so that it's immediately visible what kind of object you're working with when you don't have the initialization of the object right in front of your eyes.
+ if flavor == "gitlab": + extra_vars_d.update([ + ("gitlab_url_file", gitlab_url_file), + ("gitlab_runner_token_file", gitlab_runner_token_file), + ])
You can use a dict as argument to update(), which is more natural than a list of tuples.
^This is highly subjective I'd say, the readability doesn't really change if you don't count an extra pair of parentheses, but oki, I'll change it...
But in reality I think you can skip the check entirely and add both keys to extra_vars unconditionally: after you've fixed the issue I pointed out above, they will either be set (gitlab flavor) or None (any other flavor), and even in the latter case it's fine to pass them to Ansible as they will simply not be used.
I thought about this and I simply didn't want to pass undefined variables to ansible even if it doesn't use them, although I guess with the changes I'm currently working on it won't matter as I'm planning to stop passing visible extra vars on the command-line completely, so I think I can live with this suggestion. -- Erik Skultety
On Wed, 2020-04-08 at 12:42 +0200, Erik Skultety wrote:
On Wed, Apr 08, 2020 at 11:49:45AM +0200, Andrea Bolognani wrote:
On Tue, 2020-04-07 at 13:31 +0200, Erik Skultety wrote:
- extra_vars = json.dumps({ + extra_vars_d = {
You don't really need to change the name of the variable.
Right, although there are certain good practices like adding _l to lists and _d to dicts, so that it's immediately visible what kind of object you're working with when you don't have the initialization of the object right in front of your eyes.
I wasn't aware of these good practices, but either way we're not currently using them in lcitool so adopting them in a single case out of a thousand lines of code doesn't make a lot of sense to me. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety