Merge into trunk : trunk.add-gpg-retry : Code : curtin

Status:	Merged
Merged at revision:	445
Proposed branch:	lp://qastaging/~raharper/curtin/trunk.add-gpg-retry
Merge into:	lp://qastaging/~curtin-dev/curtin/trunk
Diff against target:	249 lines (+172/-7) 5 files modified curtin/commands/apt_config.py (+2/-1) curtin/gpg.py (+4/-4) curtin/util.py (+3/-0) tests/unittests/test_apt_source.py (+4/-2) tests/unittests/test_gpg.py (+159/-0)
To merge this branch:	bzr merge lp://qastaging/~raharper/curtin/trunk.add-gpg-retry
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2017-02-03
Scott Moser (community)		2017-02-02	Approve on 2017-02-03
Review via email: mp+316264@code.qastaging.launchpad.net

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-02-02:

#

PASSED: Continuous integration, rev:445
https://jenkins.ubuntu.com/server/job/curtin-ci/309/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/309
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/309

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/309/rebuild

review: Approve (continuous-integration)

lp://qastaging/~raharper/curtin/trunk.add-gpg-retry updated on 2017-02-02

446. By Ryan Harper on 2017-02-02: Don't hardcode retries value in gpg module.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-02-02:

#

PASSED: Continuous integration, rev:446
https://jenkins.ubuntu.com/server/job/curtin-ci/310/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/310
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/310

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/310/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-03:

#

Ryan, this looks good.
Some comments in line, possibly me just misunderstanding.

Revision history for this message

Ryan Harper (raharper) wrote on 2017-02-03:

#

Download full text (8.2 KiB)

On Thu, Feb 2, 2017 at 8:27 PM, Scott Moser <email address hidden> wrote:

> Ryan, this looks good.
> Some comments in line, possibly me just misunderstanding.
>
>
> Diff comments:
>
> >
> > === added file 'tests/unittests/test_gpg.py'
> > --- tests/unittests/test_gpg.py 1970-01-01 00:00:00 +0000
> > +++ tests/unittests/test_gpg.py 2017-02-02 22:52:32 +0000
> > @@ -0,0 +1,174 @@
> > +from unittest import TestCase
> > +from mock import call, patch
> > +import textwrap
> > +
> > +from curtin import gpg
> > +from curtin import util
> > +
> > +
> > +class TestCurtinGpg(TestCase):
> > +
> > + @patch('curtin.util.subp')
> > + def test_export_armour(self, mock_subp):
> > + key = 'DEADBEEF'
> > + expected_armour = textwrap.dedent("""
> > + -----BEGIN PGP PUBLIC KEY BLOCK-----
> > + Version: GnuPG v1
> > +
> > + deadbeef
> > + -----END PGP PUBLIC KEY BLOCK----
> > + """)
> > + mock_subp.side_effect = iter([(expected_armour, "")])
>
> what is the second side affect for ? the ""?
> I dont think subp should be called twice for this, is it ?
>

subp usually returns an (out, err) tuple; It's a habit in case we need to
do something with the error in the calling code.

>
> > +
> > + armour = gpg.export_armour(key)
> > + mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > + capture=True)
> > + self.assertEqual(expected_armour, armour)
> > +
> > + @patch('curtin.util.subp')
> > + def test_export_armour_missingkey(self, mock_subp):
> > + key = 'DEADBEEF'
> > + mock_subp.side_effect = iter([util.ProcessExecutionError()])
> > +
> > + expected_armour = gpg.export_armour(key)
> > + mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > + capture=True)
> > + self.assertEqual(None, expected_armour)
> > +
> > + @patch('curtin.util.subp')
> > + def test_recv_key(self, mock_subp):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.ubuntu.com'
> > + mock_subp.side_effect = iter([("", "")])
> > +
> > + gpg.recv_key(key, keyserver, )
>
> calling here is wierd with the following ,
>
> And again, why the 2 side_affects ? shouldn't subp just be called once?
>

It's a single side-effect with a tuple return.

the iter() is for python mock on trusty which doesn't convert the list of
side-effects into an
iterable directly.

>
> > + mock_subp.assert_called_with(["gpg", "--keyserver", keyserver,
> > + "--recv", key], capture=True,
> > + retries=None)
> > +
> > + @patch('time.sleep')
> > + @patch('curtin.util._subp')
> > + def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
> > + key = 'DEADBEEF'
> > + keyserver = 'keyserver.ubuntu.com'
> > + retries = (1, 2, 5, 10)
> > + mock_under_subp.side_effect = iter([
> > + util.ProcessExecutionError(), # 1
> > + util.ProcessExecutionError(), # 2
> > + util.ProcessExecutionError(), # 3
> > + ...

On Thu, Feb 2, 2017 at 8:27 PM, Scott Moser <smoser@ubuntu.com> wrote:

> Ryan, this looks good.
> Some comments in line, possibly me just misunderstanding.
>
>
> Diff comments:
>
> >
> > === added file 'tests/unittests/test_gpg.py'
> > --- tests/unittests/test_gpg.py       1970-01-01 00:00:00 +0000
> > +++ tests/unittests/test_gpg.py       2017-02-02 22:52:32 +0000
> > @@ -0,0 +1,174 @@
> > +from unittest import TestCase
> > +from mock import call, patch
> > +import textwrap
> > +
> > +from curtin import gpg
> > +from curtin import util
> > +
> > +
> > +class TestCurtinGpg(TestCase):
> > +
> > +    @patch('curtin.util.subp')
> > +    def test_export_armour(self, mock_subp):
> > +        key = 'DEADBEEF'
> > +        expected_armour = textwrap.dedent("""
> > +        -----BEGIN PGP PUBLIC KEY BLOCK-----
> > +        Version: GnuPG v1
> > +
> > +        deadbeef
> > +        -----END PGP PUBLIC KEY BLOCK----
> > +        """)
> > +        mock_subp.side_effect = iter([(expected_armour, "")])
>
> what is the second side affect for ? the ""?
> I dont think subp should be called twice for this, is it ?
>

subp usually returns an (out, err) tuple;  It's a habit in case we need to
do something with the error in the calling code.

>
> > +
> > +        armour = gpg.export_armour(key)
> > +        mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > +                                     capture=True)
> > +        self.assertEqual(expected_armour, armour)
> > +
> > +    @patch('curtin.util.subp')
> > +    def test_export_armour_missingkey(self, mock_subp):
> > +        key = 'DEADBEEF'
> > +        mock_subp.side_effect = iter([util.ProcessExecutionError()])
> > +
> > +        expected_armour = gpg.export_armour(key)
> > +        mock_subp.assert_called_with(["gpg", "--export", "--armour",
> key],
> > +                                     capture=True)
> > +        self.assertEqual(None, expected_armour)
> > +
> > +    @patch('curtin.util.subp')
> > +    def test_recv_key(self, mock_subp):
> > +        key = 'DEADBEEF'
> > +        keyserver = 'keyserver.ubuntu.com'
> > +        mock_subp.side_effect = iter([("", "")])
> > +
> > +        gpg.recv_key(key, keyserver, )
>
> calling here is wierd with the following ,
>
> And again, why the 2 side_affects ?  shouldn't subp just be called once?
>

It's a single side-effect with a tuple return.

the iter() is for python mock on trusty which doesn't convert the list of
side-effects into an
iterable directly.

>
> > +        mock_subp.assert_called_with(["gpg", "--keyserver", keyserver,
> > +                                      "--recv", key], capture=True,
> > +                                     retries=None)
> > +
> > +    @patch('time.sleep')
> > +    @patch('curtin.util._subp')
> > +    def test_recv_key_retry_raises(self, mock_under_subp, mock_sleep):
> > +        key = 'DEADBEEF'
> > +        keyserver = 'keyserver.ubuntu.com'
> > +        retries = (1, 2, 5, 10)
> > +        mock_under_subp.side_effect = iter([
> > +            util.ProcessExecutionError(),  # 1
> > +            util.ProcessExecutionError(),  # 2
> > +            util.ProcessExecutionError(),  # 3
> > +            util.ProcessExecutionError(),  # 4
> > +            util.ProcessExecutionError(),  # Final call
> > +        ])
> > +
> > +        with self.assertRaises(ValueError):
> > +            gpg.recv_key(key, keyserver, retries=retries)
> > +
> > +        print("_subp calls: %s" % mock_under_subp.call_args_list)
> > +        print("sleep calls: %s" % mock_sleep.call_args_list)
> > +        expected_calls = [
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
>
> since these are all the same it is shorter and more obvious (i think) with:
> expected_calls =  [mock.call(["gpg", "--keyserver" ... ])] * 5
>

Nice!  I'll do that.

>
> > +        ]
> > +        mock_under_subp.assert_has_calls(expected_calls)
> > +
> > +        expected_calls = [call(1), call(2), call(5), call(10)]
> > +        mock_sleep.assert_has_calls(expected_calls)
> > +
> > +    @patch('time.sleep')
> > +    @patch('curtin.util._subp')
> > +    def test_recv_key_retry_works(self, mock_under_subp, mock_sleep):
> > +        key = 'DEADBEEF'
>
> you dont' use key in this test.
>

?  it's passed to gpg.recv_key) and checked in the expected_calls.

>
> > +        keyserver = 'keyserver.ubuntu.com'
> > +        mock_under_subp.side_effect = iter([
> > +            util.ProcessExecutionError(),  # 1
> > +            ("", ""),
> > +        ])
> > +
> > +        gpg.recv_key(key, keyserver, retries=[1])
> > +
> > +        print("_subp calls: %s" % mock_under_subp.call_args_list)
> > +        print("sleep calls: %s" % mock_sleep.call_args_list)
> > +        expected_calls = [
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +            call(["gpg", "--keyserver", keyserver, "--recv", key],
> > +                 capture=True),
> > +        ]
> > +        mock_under_subp.assert_has_calls(expected_calls)
> > +        mock_sleep.assert_has_calls([call(1)])
> > +
> > +    @patch('curtin.util.subp')
> > +    def test_delete_key(self, mock_subp):
> > +        key = 'DEADBEEF'
> > +        mock_subp.side_effect = iter([("", "")])
> > +
> > +        gpg.delete_key(key)
> > +        mock_subp.assert_called_with(["gpg", "--batch", "--yes",
> > +                                      "--delete-keys", key],
> capture=True)
> > +
> > +    @patch('curtin.gpg.delete_key')
> > +    @patch('curtin.gpg.recv_key')
> > +    @patch('curtin.gpg.export_armour')
> > +    def test_getkeybyid(self, mock_export, mock_recv, mock_del):
> > +        key = 'DEADBEEF'
> > +        keyserver = 'my.keyserver.xyz.co.uk'
> > +
> > +        mock_export.side_effect = iter([
> > +            None,
> > +            "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > +        ])
> > +
> > +        gpg.getkeybyid(key, keyserver=keyserver)
> > +
> > +        mock_export.assert_has_calls([call(key), call(key)])
> > +        mock_recv.assert_has_calls([
> > +            call(key, keyserver=keyserver, retries=None)])
> > +        mock_del.assert_has_calls([call(key)])
> > +
> > +    @patch('curtin.gpg.delete_key')
> > +    @patch('curtin.gpg.recv_key')
> > +    @patch('curtin.gpg.export_armour')
> > +    def test_getkeybyid_exists(self, mock_export, mock_recv, mock_del):
> > +        key = 'DEADBEEF'
> > +
> > +        mock_export.side_effect = iter([
> > +            "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > +        ])
> > +
> > +        gpg.getkeybyid(key)
> > +
> > +        mock_export.assert_has_calls([call(key)])
> > +        self.assertEqual([], mock_recv.call_args_list)
> > +        self.assertEqual([], mock_del.call_args_list)
> > +
> > +    @patch('curtin.gpg.delete_key')
> > +    @patch('curtin.gpg.recv_key')
> > +    @patch('curtin.gpg.export_armour')
> > +    def test_getkeybyid_raises(self, mock_export, mock_recv, mock_del):
> > +        key = 'DEADBEEF'
> > +        keyserver = 'my.keyserver.xyz.co.uk'
> > +
> > +        mock_export.side_effect = iter([
> > +            None,
> > +            "-----BEGIN PGP PUBLIC KEY BLOCK-----",
> > +        ])
> > +        mock_recv.side_effect = iter([
> > +            ValueError("Failed to import key %s from server %s" %
> > +                       (key, keyserver)),
> > +        ])
> > +
> > +        with self.assertRaises(ValueError):
> > +            gpg.getkeybyid(key, keyserver=keyserver)
> > +
> > +        mock_export.assert_has_calls([call(key)])
> > +        mock_recv.assert_has_calls([
> > +            call(key, keyserver=keyserver, retries=None)])
> > +        mock_del.assert_has_calls([call(key)])
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.add-gpg-
> retry/+merge/316264
> You are the owner of lp:~raharper/curtin/trunk.add-gpg-retry.
>

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-03:

#

> > what is the second side affect for ? the ""?
> > I dont think subp should be called twice for this, is it ?
> >
>
> subp usually returns an (out, err) tuple; It's a habit in case we need to
> do something with the error in the calling code.

Yeah, i figured i was missing something. You are correct. Sorry for the noise.

> > > + gpg.recv_key(key, keyserver, )
> >
> > calling here is wierd with the following ,
> >
> > And again, why the 2 side_affects ? shouldn't subp just be called once?
> >
>
> It's a single side-effect with a tuple return.
>
> the iter() is for python mock on trusty which doesn't convert the list of
> side-effects into an
> iterable directly.

the subp comment was noise as mentioned above.
But why:
gpg.recv_key(key, keyserver, )
and not
gpg.recv_key(key, keyserver)

Just looks wierd.

> > you dont' use key in this test.
> >
>
> ? it's passed to gpg.recv_key) and checked in the expected_calls.

Not my best review. sorry :) Again, you are correct.

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-03:

#

go ahead and cleanup the repetitive arrays with the * operator, and then i'm good with this.
Thanks.

review: Approve

lp://qastaging/~raharper/curtin/trunk.add-gpg-retry updated on 2017-02-03

447. By Ryan Harper on 2017-02-03

Unittest cleanup after review

- Use list expansion for repeated calls
- Remove extra comma when calling gpg.recv_key

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-02-03:

#

PASSED: Continuous integration, rev:447
https://jenkins.ubuntu.com/server/job/curtin-ci/311/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-amd64/311
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/311

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/311/rebuild

review: Approve (continuous-integration)

curtin

Merge lp://qastaging/~raharper/curtin/trunk.add-gpg-retry into lp://qastaging/~curtin-dev/curtin/trunk

Commit message

Description of the change

Preview Diff

Subscribers