canonical-ubuntu-qa team mailing list archive

[Paride Legovini <mp+482575@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

A few inline comments, mostly nits apart from the one about not installing tools into ~/.local (and similar).

Diff comments:

> diff --git a/jobs/robot-framework-iso-testing/jobs.yaml b/jobs/robot-framework-iso-testing/jobs.yaml
> new file mode 100644
> index 0000000..cd1515e
> --- /dev/null
> +++ b/jobs/robot-framework-iso-testing/jobs.yaml
> @@ -0,0 +1,99 @@
> +---
> +# vim: sw=4 ts=4 et
> +
> +# QA Jenkins Jobs
> +# Copyright 2016 Canonical Ltd.
> +
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 3, as published
> +# by the Free Software Foundation.
> +
> +# This program is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranties of
> +# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
> +# PURPOSE.  See the GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License along
> +# with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# The project stanza describes all jobs and parameters
> +
> +
> +# Potential future improvements:

I know we discussed these, but now I can't remember the details. I think future us will be happy with a more explanatory comment.

> +# - throttle concurrent builds
> +# - lockable resources
> +# - exclusion
> +
> +- project:
> +    name: 'robot-framework-iso-testing'
> +    release:
> +        - 'noble'
> +    test_case:
> +        - 'entire-disk'
> +    jobs:
> +        - 'robot-framework-{release}-desktop-{test_case}'
> +
> +- job-template:
> +    name: 'robot-framework-{release}-desktop-{test_case}'
> +    description: |
> +        Runs a test case on a VM
> +    parameters:
> +      - string:
> +          name: HIT_BRANCH
> +          default: 'main'

Nit (cosmetic): in general all this yaml is a bit inconsistent about what is quoted and what is not quotes.

> +          description: branch of HIT to run tests from
> +    triggers:
> +        - pollurl:
> +            cron: '@hourly'
> +            polling-node: 'iso-testing'
> +            urls:
> +              - url: 'http://cdimage.ubuntu.com/daily-live/pending/.publish_info#robot-framework-{release}-desktop-{test_case}'
> +                check-content:
> +                  - text:
> +                      - '^{release}-.*.iso .+$'
> +              - url: 'http://cdimage.ubuntu.com/{release}/daily-live/pending/.publish_info#robot-framework-{release}-desktop-{test_case}'
> +                check-content:
> +                  - text:
> +                      - '^{release}-.*.iso .+$'
> +    wrappers:
> +      - timeout:
> +          timeout: 60
> +          fail: true
> +      - workspace-cleanup
> +      - timestamps
> +      - credentials-binding:
> +          - text:
> +              credential-id: GH_PAT
> +              variable: GH_PAT
> +    builders:
> +        - clear-artifacts
> +        - shell:
> +            command: |
> +                #!/bin/bash
> +                set -ex
> +                # determine ISO url
> +                development_release=$(distro-info --devel || echo UNKNOWN)
> +                if [ {release} = $development_release ]; then
> +                    ISO_URL="https://cdimage.ubuntu.com/daily-live/current/{release}-desktop-amd64.iso";
> +                else
> +                    ISO_URL="https://cdimage.ubuntu.com/{release}/daily-live/current/{release}-desktop-amd64.iso";
> +                fi
> +                ISO_NAME='{release}-desktop-amd64.iso'
> +                retry -d 60 -t 5 -- wget -nv -c "$ISO_URL"
> +                # clone HIT and yarf
> +                retry -t 3 -d 180 -- git clone -b $HIT_BRANCH https://oauth2:$GH_PAT@xxxxxxxxxx/canonical/hardware-installer-testing.git
> +                retry -t 3 -d 180 -- git clone -b main https://oauth2:$GH_PAT@xxxxxxxxxx/canonical/yarf.git
> +                # set up venv
> +                sudo apt install -y clang libxkbcommon-dev tesseract-ocr
> +                cd yarf/
> +                pip3 install uv
> +                ~/.local/bin/uv sync
> +                ~/.local/bin/uv pip install .[develop]

We should try to avoid installing stuff outside the job $WORKSPACE. Here, do we really need uv at all? Can't we just create the venv via `python3 -m venv`, activate it, and then install requirements.txt again with pip?

This is the only "real" request changes review comment.

> +                . .venv/bin/activate
> +                cd ../hardware-installer-testing
> +                pip install -r requirements.txt
> +                ./runner/spawn_and_run_test_suite.py --test-suite robot/test-cases/{test_case} --iso-path ../$ISO_NAME --qemu-args-json ./runner/qemu-args.json

shellcheck would complain about missing quotes around many variables, e.g. "../$ISO_NAME"

> +    publishers:
> +        - archive:
> +            artifacts: 'hardware-installer-testing/artifacts/*'
> +            allow-empty: 'true'


-- 
https://code.launchpad.net/~canonical-platform-qa/qa-jenkins-jobs/+git/qa-jenkins-jobs/+merge/482575
Your team Canonical Platform QA Team is requested to review the proposed merge of qa-jenkins-jobs:rf-vm-iso-tests into qa-jenkins-jobs:master.