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

[Cleber Rosa]

On Wed, Jul 29, 2020 at 11:16:29AM +0100, Stefan Hajnoczi wrote:
> 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
> :).
>

Yep, makes sense.

> > +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/
>

Nice catch, thanks.

> > +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/
>

Right, thanks.

> > +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//
>

You're right.  That was already bothering but I couldn't tell why.

> > 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!).

Right... Philippe has already suggested this, and you've definitely
increased its significance with the data leak example.  So yes, let's
do this rename.

Thanks for the feedback and suggestions!
- Cleber.

signature.asc
Description: PGP signature