cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

mostly looks good. a couple comments. thank you.


Diff comments:

> 
> === modified file 'cloudinit/util.py'
> --- cloudinit/util.py	2016-05-24 15:58:18 +0000
> +++ cloudinit/util.py	2016-06-07 16:05:31 +0000
> @@ -2234,3 +2234,41 @@
>      if sys.version_info[:2] < (2, 7):
>          return email.message_from_file(six.StringIO(string))
>      return email.message_from_string(string)
> +
> +
> +def gpg_export_armour(key):

maybe these in cloudinit/gpg.py ?

> +    """Export gpg key, armoured key gets returned"""
> +    (armour, _) = subp(["gpg", "--export", "--armour", key], capture=True)
> +    return armour
> +
> +
> +def gpg_recv_key(key, keyserver):
> +    """Receive gpg key from the specified keyserver"""
> +    try:
> +        subp(["gpg", "--keyserver", keyserver, "--recv", key],
> +             capture=True)
> +    except ProcessExecutionError as error:
> +        raise ValueError('Failed to import key %s from server %s - error %s' %
> +                         (key, keyserver, error))
> +
> +
> +def gpg_delete_key(key):
> +    """Delete the specified key from the local gpg ring"""
> +    subp(["gpg", "--batch", "--yes", "--delete-keys", key], capture=True)
> +
> +
> +def getkeybyid(keyid, keyserver):
> +    """get gpg keyid from keyserver"""
> +    armour = gpg_export_armour(keyid)
> +    if not armour:
> +        try:
> +            gpg_recv_key(keyid, keyserver=keyserver)
> +        except ValueError:
> +            LOG.exception('Failed to obtain gpg key %s', keyid)
> +            raise
> +
> +        armour = gpg_export_armour(keyid)
> +        # delete just imported key to leave environment as it was before

should this be a 'finally' ? so that it gets run even on ValueError ? or other Exception?

> +        gpg_delete_key(keyid)
> +
> +    return armour.rstrip('\n')
> 
> === modified file 'tests/unittests/test_handler/test_handler_apt_configure_sources_list.py'
> --- tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	2016-06-05 00:50:37 +0000
> +++ tests/unittests/test_handler/test_handler_apt_configure_sources_list.py	2016-06-07 16:05:31 +0000
> @@ -5,6 +5,24 @@
>  import os
>  import shutil
>  import tempfile
> +import socket
> +
> +# on SkipTest:
> +#  - unittest SkipTest is first preference, but it's only available
> +#    for >= 2.7
> +#  - unittest2 SkipTest is second preference for older pythons.  This
> +#    mirrors logic for choosing SkipTest exception in testtools
> +#  - if none of the above, provide custom class
> +try:
> +    from unittest.case import SkipTest
> +except ImportError:

put this stuff in tests/unittests/helpers.py

> +    try:
> +        from unittest2.case import SkipTest
> +    except ImportError:
> +        class SkipTest(Exception):
> +            """Raise this exception to mark a test as skipped.
> +            """
> +            pass
>  
>  try:
>      from unittest import mock
> @@ -20,10 +38,14 @@
>  from cloudinit.config import cc_apt_configure
>  from cloudinit.sources import DataSourceNone
>  
> +from cloudinit.distros.debian import Distro
> +
>  from .. import helpers as t_help
>  
>  LOG = logging.getLogger(__name__)
>  
> +BIN_APT = "/usr/bin/apt"

vestigial? if not, i really dislike assuming a program is in a specific path. util.which() or just 'apt'

> +
>  YAML_TEXT_CUSTOM_SL = """
>  apt_mirror: http://archive.ubuntu.com/ubuntu/
>  apt_custom_sources_list: |
> @@ -115,38 +137,41 @@
>              {'codename': '', 'primary': mirrorcheck, 'mirror': mirrorcheck})
>  
>      def test_apt_source_list_debian(self):
> -        """test_apt_source_list_debian
> -        Test rendering of a source.list from template for debian
> -        """
> +        """Test rendering of a source.list from template for debian"""
>          self.apt_source_list('debian', 'http://httpredir.debian.org/debian')
>  
>      def test_apt_source_list_ubuntu(self):
> -        """test_apt_source_list_ubuntu
> -        Test rendering of a source.list from template for ubuntu
> -        """
> +        """Test rendering of a source.list from template for ubuntu"""
>          self.apt_source_list('ubuntu', 'http://archive.ubuntu.com/ubuntu/')
>  
> -    @t_help.skipIf(True, "LP: #1589174")
> +    @staticmethod
> +    def check_connectivity(target):
> +        """try original gpg_recv_key, but allow fall back"""

comment needs fixing.
I'm really not happy about depending on external resources in a unit test.
the httpretty allows you to mock out this stuff to pretend as its there.

if we had to do this check, i'd really just rather have the user tell you OFFLINE=1 in the environment and then disable a slew of tests that way.  But depending on an external resource is just not ideal from unit test.

> +        testsock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> +        testsock.settimeout(10)
> +        try:
> +            testsock.connect((target, 80))
> +            testsock.close()
> +        except socket.error:
> +            raise SkipTest("Test skipped: no network connectivity to %s"
> +                           % target)
> +
>      def test_apt_srcl_debian_mirrorfail(self):
> -        """test_apt_source_list_debian_mirrorfail
> -        Test rendering of a source.list from template for debian
> -        """
> +        """Test rendering of a source.list from template for debian"""
> +        self.check_connectivity('httpredir.debian.org')
>          self.apt_source_list('debian', ['http://does.not.exist',
>                                          'http://httpredir.debian.org/debian'],
>                               'http://httpredir.debian.org/debian')
>  
>      def test_apt_srcl_ubuntu_mirrorfail(self):
> -        """test_apt_source_list_ubuntu_mirrorfail
> -        Test rendering of a source.list from template for ubuntu
> -        """
> +        """Test rendering of a source.list from template for ubuntu"""
> +        self.check_connectivity('archive.ubuntu.com')
>          self.apt_source_list('ubuntu', ['http://does.not.exist',
>                                          'http://archive.ubuntu.com/ubuntu/'],
>                               'http://archive.ubuntu.com/ubuntu/')
>  
>      def test_apt_srcl_custom(self):
> -        """test_apt_srcl_custom
> -        Test rendering from a custom source.list template
> -        """
> +        """Test rendering from a custom source.list template"""
>          cfg = util.load_yaml(YAML_TEXT_CUSTOM_SL)
>          mycloud = self._get_cloud('ubuntu')
>  
> 
> === modified file 'tests/unittests/test_handler/test_handler_apt_source.py'
> --- tests/unittests/test_handler/test_handler_apt_source.py	2016-06-04 02:56:45 +0000
> +++ tests/unittests/test_handler/test_handler_apt_source.py	2016-06-07 16:05:31 +0000
> @@ -17,6 +17,8 @@
>  
>  from ..helpers import TestCase
>  
> +BIN_APT = "/usr/bin/apt"

vestigial?

> +
>  EXPECTEDKEY = """-----BEGIN PGP PUBLIC KEY BLOCK-----
>  Version: GnuPG v1
>  


-- 
https://code.launchpad.net/~paelzer/cloud-init/bug-1589174-fix-tests-in-adt-env/+merge/296643
Your team cloud init development team is requested to review the proposed merge of lp:~paelzer/cloud-init/bug-1589174-fix-tests-in-adt-env into lp:cloud-init.