Re: [PATCH v2 2/2] GitLab Gating CI: initial set of jobs, documentation and scripts

[Stefan Hajnoczi]

On Wed, Jul 08, 2020 at 10:46:57PM -0400, Cleber Rosa wrote:

Awesome, thanks for creating this stuff! Minor suggestions:

> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index c1ff24370b..f8dab788ea 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -1003,3 +1003,150 @@ exercise as many corner cases as possible. It is a 
> useful test suite
>  to run to exercise QEMU's linux-user code::
>  
>    https://linux-test-project.github.io/
> +
> +CI
> +==
> +
> +QEMU has configurations enabled for a number of different CI services.
> +The most update information about them and their status can be found
> +at::
> +
> +   https://wiki.qemu.org/Testing/CI
> +
> +Gating CI
> +----------
> +
> +A Pull Requests will only to be merged if they successfully go through
> +a different set of CI jobs.  GitLab's CI is the service/framework used

s/A Pull Requests/Pull Requests/
s/will only to be merged/will only be merged/

I suggest simplifying the first sentence:

  Code is only merged after passing the "gating" set of CI jobs.

Whether they are called Pull Requests or Merge Requests shouldn't matter
:).

> +for executing the gating jobs.
> +
> +The architecture of GitLab's CI service allows different machines to be
> +setup with GitLab's "agent", called gitlab-runner, which will take care

s/setup/set up/ throughout this document
https://grammarist.com/spelling/set-up-vs-setup/

> +of running jobs created by events such as a push to a branch.
> +
> +Even though gitlab-runner can execute jobs on environments such as
> +containers, this initial implementation assumes the shell executor is
> +used, effectively running jobs on the same machine (be them physical

s/them/they/

> +or virtual) the gitlab-runner agent is running.  This means those

s/the/where the/

> +machines must be setup in advance, with the requirements matching the
> +jobs expected to be executed there.
> +
> +Machine configuration for gating jobs
> +-------------------------------------
> +
> +The GitLab's CI architecture allows different parties to provide
> +different machines that will run different jobs.  At this point, QEMU
> +will deploy a limited set of machines and jobs.  Documentation and/or
> +scripts to setup those machines is located under::
> +
> +  scripts/ci/setup
> +
> +Ansible playbooks have been provided to perform two different tasks
> +related to setting gitlab-runner and the build environment.

s/setting/setting up/

> +
> +Other organizations involved in QEMU development may, in the near
> +future, contribute their own setup documentation/scripts under

Comments about relative time lack context in a long-lived document like
this one:
s/in the near future//

> diff --git a/scripts/ci/setup/build-environment.yml 
> b/scripts/ci/setup/build-environment.yml
> new file mode 100644
> index 0000000000..89b35386c7
> --- /dev/null
> +++ b/scripts/ci/setup/build-environment.yml
> @@ -0,0 +1,217 @@
> +---
> +- name: Installation of basic packages to build QEMU
> +  hosts: all
> +  vars_files:
> +    - vars.yml
> +  tasks:
> +    - name: Install basic packages to build QEMU on Ubuntu 18.04/20.04
> +      apt:
> +        update_cache: yes
> +        # This matches the packages on 
> tests/docker/Dockerfiles/ubuntu1804.docker

These comments will not age well :). If you really want to leave a note
then I suggest "Originally from
tests/docker/Dockerfiles/ubuntu1804.docker".

> diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> new file mode 100644
> index 0000000000..8bb7ba6b33
> --- /dev/null
> +++ b/scripts/ci/setup/inventory
> @@ -0,0 +1,2 @@
> +[local]
> +localhost
> diff --git a/scripts/ci/setup/vars.yml b/scripts/ci/setup/vars.yml

Perhaps this file can be called vars.yml.template and an entry for
vars.yml can be added to .gitignore. A file that needs local editing
should not be commited to git in-place. Otherwise it's easy to
accidentally commit the local changes to git (and expose the private
GitLab token!).

signature.asc
Description: PGP signature