[libvirt-python PATCH 2/3] gitlab: introduce CI jobs testing git master & distro libvirt

[Andrea Bolognani]

On Wed, 2020-04-29 at 14:36 +0100, Daniel P. Berrangé wrote:
> +++ b/.gitlab-ci.yml
> @@ -0,0 +1,172 @@
> +.container_job_template: &container_job_definition
> +  image: docker:stable
> +  stage: containers
> +  services:
> +    - docker:dind
> +  before_script:
> +    - export TAG="${CI_REGISTRY_IMAGE}/buildenv-${NAME}:latest"
> +    - export COMMON_TAG="${CI_REGISTRY}/libvirt/libvirt-perl/buildenv-${NAME}:latest"

Since we're now storing images directly under the project's namespace
on registry.gitlab.com and not somewhere more public like Quay, where
people could conceivably look for a container image that can be used
to *run* libvirt and friends instead of build them, I think we can
safely drop the buildenv- prefix.

> +    - docker info
> +    - docker login registry.gitlab.com -u ${CI_REGISTRY_USER} -p ${CI_REGISTRY_PASSWORD}
> +  script:
> +    - docker pull ${TAG} || docker pull ${COMMON_TAG} || true
> +    - docker build --cache-from ${TAG} --cache-from ${COMMON_TAG} --tag ${TAG} -f ci/libvirt-${NAME}.dkr ci

Using a .dkr extension for Dockerfiles is very non-standard, and vim
for example doesn't recognize it as a Dockerfile because of that.
Please use either $OS.Dockerfile or, even better, $OS/Dockerfile,
which would avoid the need to use the -f option in the build command.

> +.build_git_job_template: &build_git_job_definition
> +  stage: build
> +  before_script:
> +    - export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
> +    - export SCRATCH_DIR="/tmp/scratch"
> +    - export VROOT="${SCRATCH_DIR}/vroot"
> +    - export LD_LIBRARY_PATH="${VROOT}/lib"
> +    - export PATH="${PATH}:${VROOT}/bin"
> +    - export PKG_CONFIG_PATH="${VROOT}/lib/pkgconfig"
> +  script:
> +    - pushd ${PWD}
> +    - mkdir -p ${SCRATCH_DIR}
> +    - cd ${SCRATCH_DIR}
> +    - git clone --depth 1 https://gitlab.com/libvirt/libvirt.git src
> +    - mkdir build
> +    - cd build
> +    - ../src/autogen.sh --prefix=${VROOT} --without-libvirtd
> +    - make install
> +    - popd
> +    - ${PYTHON} setup.py build
> +    - ${PYTHON} setup.py test

Style-wise, all the scripts introduced in this patch contain a lot of
unnecessary curly braces and could use a bit more quoting, so it'd be
nice (but not blocking) if you could clean that up.

> +.build_dist_job_template: &build_dist_job_definition
> +  stage: build
> +  before_script:
> +    - export MAKEFLAGS="-j$(getconf _NPROCESSORS_ONLN)"
> +    - export TEST_MAINTAINER=1

Is TEST_MAINTAINER a thing for libvirt-python? The only reference to
it I could find in the libvirt-ci repo is in the job definition for
libvirt-perl.

> +ctr-centos-7:
> +  <<: *container_job_definition
> +  variables:
> +    NAME: centos-7

Not to keen on the ctr- prefix. I understand you're trying to keep
the job names short so that they all fit in the tiny boxes in the
pipeline overview page, but I don't think that should be an absolute
requirement considering that the first thing you're going to do when
you see a failure there is to click on the name of the job to see
more details, and when you do so the full name will be displayed
quite prominently in the top-right corner in a way that's actually
readable, unlike the cut-off low-constrast version that's on the
pipeline overview page...

> +build-git-centos-8:
> +  <<: *build_git_job_definition
> +  image: ${CI_REGISTRY_IMAGE}/buildenv-centos-8:latest

... and these slightly longer names already get cut off anyway.

[...]
> +++ b/ci/refresh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +if test -z "$1"
> +then
> +  echo "syntax: $0 PATH-TO-LCITOOL"
> +  exit 1
> +fi

I'd also check that whatever the path is pointing to is executable.

The indentation is also off - two spaces instead of four.

> +LCITOOL=$1
> +
> +HOSTS=$(${LCITOOL} hosts | grep -v freebsd)
> +
> +for host in ${HOSTS}
> +do
> +    if test "$host" == "libvirt-centos-8"

== is a bashism, use the portable = instead.

> +    then
> +	${LCITOOL} dockerfile $host libvirt-minimal,libvirt-dist,libvirt-python > $host.dkr
> +    else
> +	${LCITOOL} dockerfile $host libvirt-dist,libvirt-python > $host.dkr
> +    fi
> +done

The indentation is off here, I think some tabs sneaked in.


We should also add a short README that tells people what this
directory is all about and how to make changes to it, but we can come
up with something that works for all repos and push it everywhere as
a follow-up enhancement.


Overall, aside from the nits pointed out above, the idea is solid and
the implementation looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization