Re: [libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage

On Mon, Jan 31, 2022 at 07:01:01PM +0100, Erik Skultety wrote: Using == with test is a bashism, please stick to the portable version even though it's very likely that the script will ultimately run under bash. I suggest changing this to something along the lines of - for daemon in $DAEMONS; do log_outputs="file:/var/log/libvirt/${daemon}.log" log_filters="3:remote 4:event 3:util.json 3:util.object 3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*" sed -Ei -e "s;^#*\\s*log_outputs\\s*=.*$;log_outputs=\"$log_outputs\";g" \ -e "s;^#*\\s*log_filters\\s*=.*$;log_filters=\"$log_filters\";g" \ "src/remote/${daemon}.conf.in" # ... done Advantages: * saves one call to sed; * doesn't need the awkward quoting for slashes in paths; * produces key="value" which is consistent with the existing contents of the configuration files (even though the parser will accept single quotes too); * handles slight, semantically-irrelevant changes to the contents of the configuration files such as whitespace being added; * doesn't unnecessarily use captures; * avoids excessively long lines. Note that I have adjusted the value of log_filters to match what is recommended in https://libvirt.org/kbase/debuglogs.html#less-verbose-logging-for-qemu-vms but maybe there's a reason you had picked a different set of filters. Also note that I haven't actually tested that the above works correctly O:-) Unnecessary empty line here. Same as in the previous patch, I'd like for names of directories to end with a slash. IIUC from the documentation and from reading around, using strategy:depend will cause the local job to reflect the status of the triggered pipeline. So far so good. What I am unclear about is, is there any guarantee that using artifact:true with a job from an external project's pipeline will expose the artifacts from the job that was executed as part of the specific pipeline that we've just triggered ourselves as opposed to some other pipeline that might have already been completed in the past of might have completed in the meantime? Taking a step back, why exactly are we triggering a rebuild of libvirt-perl in the first place? Once we change that project's pipeline so that RPMs are published as artifacts, can't we just grab the ones from the latest successful pipeline? Maybe you've already explained why you did things this way and I just missed it! This variable doesn't seem to be used anywhere, so I assume it's a leftover from development. Maybe you tried to implement the .test template so that using it didn't require as much repetition and unfortunately it didn't work out? IIUC this is used both by the GitLab scheduler to pick suitable nodes on which to execute the job (our own hardware in this case) and also by the runner to decide which template to use for the VM. So shouldn't this be more specific? I would expect something like tags: - centos-stream-8-vm or tags: - centos-stream-8 - vm Unlike Dan, I actually think it makes sense to have these as a separate stage. I'd call it integration_tests and list it last though. But admittedly this is entirely in bikeshedding territory and I'm fine with the current solution or the one Dan proposed too :) -- Andrea Bolognani / Red Hat / Virtualization