curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+407871@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Basically fine, a few comments/questions though.

Diff comments:

> diff --git a/tests/vmtests/test_python_apt.py b/tests/vmtests/test_python_apt.py
> new file mode 100644
> index 0000000..4162608
> --- /dev/null
> +++ b/tests/vmtests/test_python_apt.py
> @@ -0,0 +1,42 @@
> +# This file is part of curtin. See LICENSE file for copyright and license info.
> +
> +from aptsources.sourceslist import SourceEntry
> +
> +from . import VMBaseClass
> +from .releases import base_vm_classes as relbase
> +
> +
> +class TestPythonApt(VMBaseClass):
> +    """TestPythonApt - apt sources manipulation with python{,3}-apt"""
> +    test_type = 'config'
> +    conf_file = "examples/tests/apt_source_custom.yaml"
> +
> +    def test_python_apt(self):
> +        """test_python_apt - Ensure the python-apt package is available"""
> +
> +        line = 'deb http://us.archive.ubuntu.com/ubuntu/ hirsute main'
> +
> +        self.assertEqual(line, str(SourceEntry(line)))

This seems to be a test of the test environment really? In which case actually running the vmtests seems a little wasteful. If you're planning to extend this file to have actual useful test content later, then that's fine I guess.

> +
> +
> +class XenialTestPythonApt(relbase.xenial, TestPythonApt):
> +    __test__ = True
> +
> +
> +class BionicTestPythonApt(relbase.bionic, TestPythonApt):
> +    __test__ = True
> +
> +
> +class FocalTestPythonApt(relbase.focal, TestPythonApt):
> +    __test__ = True
> +
> +
> +class HirsuteTestPythonApt(relbase.hirsute, TestPythonApt):
> +    __test__ = True
> +
> +
> +class ImpishTestPythonApt(relbase.impish, TestPythonApt):
> +    __test__ = True
> +
> +
> +# vi: ts=4 expandtab syntax=python
> diff --git a/tools/noproxy b/tools/noproxy
> deleted file mode 100644
> index 911cc7a..0000000
> --- a/tools/noproxy
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -#!/usr/bin/env python

I think you covered this in a different MP or commit message or something but can you add some rationale for removing this to the commit message?

> -# This file is part of curtin. See LICENSE file for copyright and license info.
> -
> -# clean http_proxy variables from environment as they make httpretty
> -# fail, but may be in the environment for reasons such as pip install
> -import os
> -import sys
> -
> -for k in ('http_proxy', 'https_proxy', 'HTTP_PROXY', 'HTTPS_PROXY'):
> -    if k in os.environ:
> -        del os.environ[k]
> -
> -os.execvpe(sys.argv[1], sys.argv[1:], os.environ)
> -
> -# vi: ts=4 expandtab syntax=python
> diff --git a/tools/vmtest-system-setup b/tools/vmtest-system-setup
> index b9185db..30a8246 100755
> --- a/tools/vmtest-system-setup
> +++ b/tools/vmtest-system-setup
> @@ -11,6 +11,13 @@ case "$(uname -m)" in
>    ppc*) qemu="qemu-system-ppc";;
>    s390x) qemu="qemu-system-s390x";;
>  esac
> +
> +case "$rel" in

Nothing else in this file appears to use $rel, so maybe change it to series=$(lsb_release -sr) and use that instead, which has the bonus feature that it can be compared safely with lexicographical operators.

> +    "t"|"u"|"x"*) python_apt="python-apt";;

this will match strings like "t" and "xylophone" but not "trusty", right? I doubt that's what you want, but as above I think you should do this differently anyway :)

> +    "b"|"f"*) python_apt="python-apt python3-apt";;
> +    *) python_apt="python3-apt";;
> +esac
> +
>  DEPS=(
>    cloud-image-utils
>    make


-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/407871
Your team curtin developers is subscribed to branch curtin:master.