canonical-ubuntu-qa team mailing list archive

[Paride Legovini <mp+465334@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Some inline comments.

Diff comments:

> diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-nova-image-with-proposed-package b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-nova-image-with-proposed-package
> index e48f3f4..1ac3554 100755
> --- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-nova-image-with-proposed-package
> +++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/tools/create-nova-image-with-proposed-package
> @@ -67,7 +78,7 @@ def setup_image(image_path, source):
>              image_path,
>              "chroot",
>              "_MOUNTPOINT_",
> -            "/bin/sh",
> +            "/bin/bash",

Why? Sticking with plain sh when possible is a good idea.

>          ],
>          stdin=subprocess.PIPE,
>      )
> @@ -76,30 +87,37 @@ def setup_image(image_path, source):
>      # pylint: disable=line-too-long
>      img_shell.stdin.write(
>          (
> -            """
> +            f"""
>  set -e
>  echo '* Creating policy-rc.d'
>  printf '#!/bin/sh\\nexit 101\\n' > /usr/sbin/policy-rc.d
>  chmod 755 /usr/sbin/policy-rc.d
>  echo '* Generating apt sources for -proposed:'
> -sed -rn 's/^(deb|deb-src) +(\[.*\] *)?([^ ]*(ubuntu.com|debian.org|ftpmaster)[^ ]*) +([^ -]+) +(.*)$/\\1 \\2\\3 \\5-proposed \\6/p' /etc/apt/sources.list `ls /etc/apt/sources.list.d/*.list 2>/dev/null|| true` > /etc/apt/sources.list.d/proposed.list
> +
> +touch /etc/apt/sources.list.d/proposed.list

touch not needed

> +echo "deb     http://ftpmaster.internal/ubuntu/ {release}-proposed main restricted universe multiverse" > /etc/apt/sources.list.d/proposed.list
> +echo "deb-src http://ftpmaster.internal/ubuntu/ {release}-proposed main restricted universe multiverse" >> /etc/apt/sources.list.d/proposed.list
> +
>  cat /etc/apt/sources.list.d/proposed.list
>  
>  echo '* apt-get update'
> -apt-get update
> +apt -o "Dir::Etc::sourcelist=/etc/apt/sources.list.d/proposed.list" update

apt is not meant to be used in scripts, stick with apt-get.

>  
> -echo '* Determining which binaries of source %(src)s are installed and have a new version...'
> -SRCS=$(python3 -c 'import sys, apt; src=sys.argv[1]; [print(pkg.name) for pkg in apt.Cache() if pkg.installed and pkg.candidate.source_name == src and pkg.candidate.version > pkg.installed.version]' %(src)s)
> -echo "$SRCS"
> +#echo '* Determining which binaries of source {source} are installed and have a new version...'
> +#SRCS=$(python3 -c 'import sys, apt; src=sys.argv[1]; [print(pkg.name) for pkg in apt.Cache() if pkg.installed and pkg.candidate.source_name == src and pkg.candidate.version > pkg.installed.version]' %(src)s)
> +#echo "$SRCS"
>  
>  echo '* Install the above packages'
> -DEBIAN_FRONTEND=noninteractive apt-get install -y $SRCS
> +DEBIAN_FRONTEND=noninteractive apt-get install -y {source}

This is wrong: {source} is a source package, while we want to install the binary packages that were built from that source package. In the case of src:base-files this worked because it happens to build a binary package with the same name (base-files), but that's not always the case.

The logic you commented out ("Determining which binaries ...") was meant to do that. Better: it was meant to only do upgrades. This is what we want.

>  
> -echo '* Cleaning up'
> +echo '*Cleaning up apt'

Nit: little formatting error (missing space)

> +rm /etc/apt/sources.list.d/proposed.list
> +apt update
>  apt-get clean
> +
> +echo '* Cleaning up'
>  rm -f /etc/machine-id /usr/sbin/policy-rc.d
>     """
> -            % {"src": source}
>          ).encode()
>      )
>  
> @@ -111,14 +129,16 @@ rm -f /etc/machine-id /usr/sbin/policy-rc.d
>  # main
>  #
>  
> -if len(sys.argv) != 3:
> +if len(sys.argv) != 4:
>      sys.stderr.write(
> -        "Usage: %s <image RE> <proposed source package name>\n" % sys.argv[0]
> +        "Usage: %s <image RE> <proposed source package name> <release-codename>\n"

I get that we can't use /etc/os-release to infer the release, as that's exactly the thing that bitten us. But maybe we can extract the release from the image name? We assume that to have a well defined structure in many places.

This would prevent us from having an extra argument.

> +        % sys.argv[0]
>      )
>      sys.exit(1)
>  
>  image_re = re.compile(sys.argv[1])
>  source = sys.argv[2]
> +release = sys.argv[3]
>  glance = get_glance()
>  latest = find_latest_image(image_re)
>  


-- 
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/465334
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:proposed-package-images into autopkgtest-cloud:master.