cloud-init-dev team mailing list archive

[ChristianEhrhardt <christian.ehrhardt@xxxxxxxxxxxxx>]

Thanks, addressed all you mentioned - will throw it through more testing now.

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):

Yep, should help code being more readable - done.

> +    """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

Good catch,
in fact I've given most of it more thought catching more potential Exceptions and adding LOG (debug/warn) as appropriate.
Then the finally makes sense and is implemented now.

> +        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:

Done

> +    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"

Asking my translator I'm not sure if this is an offense :-)

But on the good side, since the last changes to make things CentOS compatible we don't need this definition anymore - so removing is even better.

> +
>  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"""

It seems that as much as you "dislike depending on external resources" I'd like to "use the real resources if possible" to make the test more real.
Given the current implementation it is not really "Depending" but trying to us and skipping if not - so it doesn't fail if the external resource is not there.
I really like it this way.

But then your vote and experience in all of this counts more so I'll have to mock it then.
Putting my sadness of not hitting the real server away and taking this as a chance to touch httpretty ... :-)
I read into it, but then realized that this here isn't http at all.

Eventually the important part "tested" here is util.is_resolvable which means socket.getaddrinfo.
After understanding that it simplifies to "mock util.is_resolvable" which I have done in the updated MP that will follow.
(That also gets totally rid of the Skip class I moved before)

> +        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')
>  


-- 
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.