Merge lp:~cjohnston/ubuntu-ci-services-itself/fix-flake8-issues into lp:ubuntu-ci-services-itself

[Chris Johnston]

Fix misc flake8 issues around the project

We should adhere to pep8

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:235
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/146/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/146/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:235
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/147/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/147/rebuild

review: Approve (continuous-integration)

[Ursula Junque (ursinha)]

The changes look good (can't disagree with flake8 cleanup :)). I see that all cupstream2distro reused code needs pep8 love, so might be too much for this branch, but I found three worth fixing in this branch:

branch-source-builder/watch_ppa.py:174:1: F821 undefined name 'UPLOAD_LIST'
ci-utils/ci_utils/nova_client/__init__.py:75:1: F841 local variable 'id' is assigned to but never used
cli/tests/test_utils.py:34:1: E302 expected 2 blank lines, found 1

Other than that that's great.

[Vincent Ladeuil (vila)]

> We should adhere to pep8

+1

Yet, it's not good if it's not automatic. It's on my TODO list as part of our test story but not at the top yet (ucitests provides a base class that can be used for that but does not work (yet) on precise).

...

Now that I think about it, I should just skip that on precise, I don't think there are a lot of us developing on precise ... will make a MP later.

...

In fact, I already did almost all that is needed in ucitests-0.0.8, so less work than I think.

review: Approve

[Chris Johnston (cjohnston)]

On Thu, Feb 13, 2014 at 11:00 AM, Ursula Junque <email address hidden> wrote:

> The changes look good (can't disagree with flake8 cleanup :)). I see that
> all cupstream2distro reused code needs pep8 love, so might be too much for
> this branch, but I found three worth fixing in this branch:
>

Since cupstream2distro was a direct copy and paste, I disagree with making
changes to it. If we want to fix them, I think they should be fixed
upstream first and then brought to us, otherwise making changes across both
will become more difficult. I also didn't even look at any of the charms
other than webui for that same reason.

>
> branch-source-builder/watch_ppa.py:174:1: F821 undefined name 'UPLOAD_LIST'
> ci-utils/ci_utils/nova_client/__init__.py:75:1: F841 local variable 'id'
> is assigned to but never used
> cli/tests/test_utils.py:34:1: E302 expected 2 blank lines, found 1
>
>
cli was fixed in a previous MP. UPLOAD_LIST and id I was going to talk to
the developers about. UPLOAD_LIST looks like there is part of a story
missing.

> Other than that that's great.

[Ursula Junque (ursinha)]

> On Thu, Feb 13, 2014 at 11:00 AM, Ursula Junque <email address hidden> wrote:
>
> > The changes look good (can't disagree with flake8 cleanup :)). I see that
> > all cupstream2distro reused code needs pep8 love, so might be too much for
> > this branch, but I found three worth fixing in this branch:
> >
>
> Since cupstream2distro was a direct copy and paste, I disagree with making
> changes to it. If we want to fix them, I think they should be fixed
> upstream first and then brought to us, otherwise making changes across both
> will become more difficult. I also didn't even look at any of the charms
> other than webui for that same reason.

I'm not sure they're only copy&paste, from fginther's MP it seems the code was copied and adapted, so it's unclear we're bringing upstream changes to our code. But that's a separate thing, I think, as it needs further discussion.

>
>
> >
> > branch-source-builder/watch_ppa.py:174:1: F821 undefined name 'UPLOAD_LIST'
> > ci-utils/ci_utils/nova_client/__init__.py:75:1: F841 local variable 'id'
> > is assigned to but never used
> > cli/tests/test_utils.py:34:1: E302 expected 2 blank lines, found 1
> >
> >
> cli was fixed in a previous MP. UPLOAD_LIST and id I was going to talk to
> the developers about. UPLOAD_LIST looks like there is part of a story
> missing.
>
>
> > Other than that that's great.

=== modified file 'branch-source-builder/run_worker'
--- branch-source-builder/run_worker	2014-02-11 20:57:36 +0000
+++ branch-source-builder/run_worker	2014-02-12 21:16:08 +0000
@@ -32,6 +32,7 @@
 from ci_utils import amqp_utils
 TIME_BETWEEN_CHECKS = 60
 
+
 def on_message(msg):
     log.info('on_message: {}'.format(msg.body))
     params = json.loads(msg.body)

=== modified file 'branch-source-builder/upload_package.py'
--- branch-source-builder/upload_package.py	2014-02-07 17:32:37 +0000
+++ branch-source-builder/upload_package.py	2014-02-12 21:16:08 +0000
@@ -17,7 +17,6 @@
 import argparse
 import atexit
 from dput.changes import parse_changes_file
-import json
 import logging
 import os
 import re
@@ -43,9 +42,9 @@
 
 
 def cleanup(directory):
-     if os.path.exists(directory) and os.path.isdir(directory):
-         logging.info('Removing temp directory: {}'.format(directory))
-         shutil.rmtree(directory)
+    if os.path.exists(directory) and os.path.isdir(directory):
+        logging.info('Removing temp directory: {}'.format(directory))
+        shutil.rmtree(directory)
 
 
 def create_temp_dir():

=== modified file 'branch-source-builder/watch_ppa.py'
--- branch-source-builder/watch_ppa.py	2014-02-07 17:32:37 +0000
+++ branch-source-builder/watch_ppa.py	2014-02-12 21:16:08 +0000
@@ -80,7 +80,7 @@
     # Get archs available and archs to ignore
     (available_archs_in_ppa,
      arch_all_arch) = launchpadmanager.get_available_and_all_archs(
-         lp_series, monitored_ppa)
+        lp_series, monitored_ppa)
     (archs_to_eventually_ignore,
      archs_to_unconditionally_ignore) = launchpadmanager.get_ignored_archs()
     logging.info('Arches available in ppa: {}'.format(available_archs_in_ppa))

=== modified file 'charms/precise/webui/hooks/hooks.py'
--- charms/precise/webui/hooks/hooks.py	2014-01-27 21:44:48 +0000
+++ charms/precise/webui/hooks/hooks.py	2014-02-12 21:16:08 +0000
@@ -2,7 +2,6 @@
 
 import distutils.dir_util
 import grp
-import json
 import os
 import pwd
 import re

=== modified file 'ci-utils/ci_utils/nova_client/__init__.py'
--- ci-utils/ci_utils/nova_client/__init__.py	2014-01-29 23:01:46 +0000
+++ ci-utils/ci_utils/nova_client/__init__.py	2014-02-12 21:16:08 +0000
@@ -38,19 +38,18 @@
         self.nova = novaclient.Client(**creds)
         image = self.nova.images.find(name=imagename)
         inst_flavor = self.nova.flavors.find(name=flavor)
-        SSH_KEYNAME=os.path.expanduser('~/.ssh/id_rsa')
+        SSH_KEYNAME = os.path.expanduser('~/.ssh/id_rsa')
         if not os.path.exists(SSH_KEYNAME):
             subprocess.call(
-                ['ssh-keygen','-t','rsa','-f',SSH_KEYNAME,'-N',''])
+                ['ssh-keygen', '-t', 'rsa', '-f', SSH_KEYNAME, '-N', ''])
         if not self.nova.keypairs.findall(name="sshkey"):
             with open(SSH_KEYNAME+'.pub') as sshkey:
-                self.key=self.nova.keypairs.create(
+                self.key = self.nova.keypairs.create(
                     name="sshkey", public_key=sshkey.read())
-        self.instance=self.nova.servers.create(
+        self.instance = self.nova.servers.create(
             name="test", image=image, flavor=inst_flavor, key_name="sshkey")
         self.ip = self._get_ip()
 
-
     def _wait_for_instance(self):
         status = self.instance.status
         #Try for up to 5 minutes to create the instance
@@ -63,19 +62,17 @@
         if status == 'BUILD':
             raise NovaClientException("Instance creation timed out")
 
-
     def _refresh_instance(self):
-        id=self.instance.id
+        id = self.instance.id
         try:
             #You have to re-get the instance for it to update
             self.instance = self.nova.servers.get(id)
         except ConnectionError:
             log.warn("Received connection error, retrying")
 
-
     def _get_ip(self):
         timeout = time.time()+300
-        id=self.instance.id
+        id = self.instance.id
         ip = None
         while not ip and time.time() < timeout:
             try:
@@ -83,7 +80,7 @@
                 self._refresh_instance()
                 #The value first instance in networks should contain the
                 #value we need
-                ip=self.instance.networks.values()[0][0]
+                ip = self.instance.networks.values()[0][0]
             except IndexError:
                 log.info("Waiting for IP to become available")
                 time.sleep(5)
@@ -92,36 +89,31 @@
         log.info("IP is %s" % ip)
         return ip
 
-
     def runcmd(self, cmd):
-        sshopts = ['-o','UserKnownHostsFile=/dev/null','-o ',
+        sshopts = ['-o', 'UserKnownHostsFile=/dev/null', '-o ',
                    'StrictHostKeyChecking=no']
         cmd = ['ssh'] + ['ubuntu@'+self.ip] + sshopts + cmd.split(' ')
         return subprocess.check_output(cmd)
 
-
     def scp(self, src, dest, recurse=False):
-        sshopts = ['-o','UserKnownHostsFile=/dev/null','-o ',
+        sshopts = ['-o', 'UserKnownHostsFile=/dev/null', '-o ',
                    'StrictHostKeyChecking=no']
         if recurse:
             sshopts.append['-r']
-        cmd = ['scp'] + ['ubuntu@'+self.ip] + sshopts + [src, dest]
-
+        ['scp'] + ['ubuntu@'+self.ip] + sshopts + [src, dest]
 
     def delete(self):
         self.instance.delete()
 
-
     def _get_nova_creds(self, config):
         try:
             creds = {'auth_url': config['auth_url'],
-                    'username': config['auth_user'],
-                    'api_key': config['auth_password'],
-                    'project_id': config['auth_tenant_name'],
-                    'region_name': config['auth_region']}
+                     'username': config['auth_user'],
+                     'api_key': config['auth_password'],
+                     'project_id': config['auth_tenant_name'],
+                     'region_name': config['auth_region']}
         except KeyError:
             raise self.NovaClientException(
                 "Missing or invalid authentication info."
             )
         return creds
-

=== modified file 'ci-utils/ci_utils/tests/test_jenkins.py'
--- ci-utils/ci_utils/tests/test_jenkins.py	2014-02-07 22:09:36 +0000
+++ ci-utils/ci_utils/tests/test_jenkins.py	2014-02-12 21:16:08 +0000
@@ -15,8 +15,6 @@
 
 import json
 import mock
-import os
-import sys
 import unittest
 
 from ci_utils import jenkins_utils

=== modified file 'ci-utils/ci_utils/tests/test_tmpdir.py'
--- ci-utils/ci_utils/tests/test_tmpdir.py	2014-01-10 16:47:39 +0000
+++ ci-utils/ci_utils/tests/test_tmpdir.py	2014-02-12 21:16:08 +0000
@@ -22,6 +22,6 @@
 class TestTmpDir(unittest.TestCase):
     def test_tmpdir(self):
         with TmpDir() as tmpdir:
-            dir_created=tmpdir
+            dir_created = tmpdir
             self.assertTrue(os.path.exists(dir_created))
         self.assertFalse(os.path.exists(dir_created))

=== modified file 'juju-deployer/test_update.py'
--- juju-deployer/test_update.py	2014-02-04 15:58:00 +0000
+++ juju-deployer/test_update.py	2014-02-12 21:16:08 +0000
@@ -132,7 +132,7 @@
     def test_get_branches_and_revnos(self):
         branch_url = self.get_branch_url()
         current = self.branch.revno()
-        y = self.get_service_yaml(current -1)
+        y = self.get_service_yaml(current - 1)
         path = os.path.join(self.temp_dir, 'foo.yaml')
         with open(path, 'w') as fp:
             fp.write(yaml.safe_dump(y))
@@ -141,7 +141,7 @@
             actual = update.get_branches_and_revnos(
                 [path], True)
             msg = '{} is out of date (currently {}, latest {})'
-            msg = msg.format(branch_url, current -1, current)
+            msg = msg.format(branch_url, current - 1, current)
             warn.assert_called_with(msg)
         self.assertEqual(actual, {branch_url: current})
 

=== modified file 'juju-deployer/update.py'
--- juju-deployer/update.py	2014-02-04 15:58:00 +0000
+++ juju-deployer/update.py	2014-02-12 21:16:08 +0000
@@ -43,7 +43,6 @@
 plugin.load_plugins()
 
 
-
 class UpdateArgParser(argparse.ArgumentParser):
     """A parser for the uci-run-tests script."""
 
@@ -140,7 +139,7 @@
 
 def set_branches_and_revnos(paths, branches):
     """Update the yaml files with the new revnos.
-    
+
     We cannot use pyyaml for this, as it doesn't preserve order or preserve
     comments.
     """
@@ -163,6 +162,7 @@
             with open(cfg, 'w') as fp:
                 fp.write(new_body)
 
+
 def main(args=None):
     if args is None:
         args = sys.argv[1:]
@@ -174,7 +174,7 @@
     paths = list(get_deployer_configs(deployer_dir))
 
     if ns.assert_pinned:
-        all_pinned =  ensure_all_branches_are_pinned(paths)
+        all_pinned = ensure_all_branches_are_pinned(paths)
         return not bool(all_pinned)
     else:
         branches = get_branches_and_revnos(paths, ns.check)

=== modified file 'lander/bin/lander_archiver.py'
--- lander/bin/lander_archiver.py	2014-02-06 18:50:05 +0000
+++ lander/bin/lander_archiver.py	2014-02-12 21:16:08 +0000
@@ -18,7 +18,6 @@
 import logging
 import os
 import sys
-import urllib2
 import yaml
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '../../ci-utils'))

=== modified file 'ppa-assigner/ppa_assigner/management/commands/integration_test.py'
--- ppa-assigner/ppa_assigner/management/commands/integration_test.py	2013-12-23 19:46:06 +0000
+++ ppa-assigner/ppa_assigner/management/commands/integration_test.py	2014-02-12 21:16:08 +0000
@@ -41,9 +41,10 @@
                          default=%(default)s'''),
         make_option('--pocket', dest='pocket', default='Release',
                     help='Pocket to copy package in. default=%(default)s'),
-        make_option('--archive', dest='archive',
-                    default='https://api.launchpad.net/1.0/ubuntu/+archive/primary',
-                    help='Archive for source package. defafult=%(default)s'),
+        make_option(
+            '--archive', dest='archive',
+            default='https://api.launchpad.net/1.0/ubuntu/+archive/primary',
+            help='Archive for source package. defafult=%(default)s'),
     )
 
     def handle(self, *args, **options):

=== modified file 'run-tests'
--- run-tests	2014-02-04 14:50:46 +0000
+++ run-tests	2014-02-12 21:16:08 +0000
@@ -87,8 +87,8 @@
                   'branch-source-builder',
                   'image-builder',
                   'lander',
-#                  'ppa-assigner',
-#                  'ticket_system',
+                  # 'ppa-assigner',
+                  # 'ticket_system',
                   'test_runner',
                   ]
     loader = loaders.Loader()

=== modified file 'test_runner/tstrun/testbed.py'
--- test_runner/tstrun/testbed.py	2014-02-06 11:23:26 +0000
+++ test_runner/tstrun/testbed.py	2014-02-12 21:16:08 +0000
@@ -294,8 +294,6 @@
 # 'auth_testbed_sshkey' can be defined there if you don't use ~/.ssh/id_rsa.
 
 def test_print_ip():
-    precise_image = ('ubuntu-released/ubuntu-precise-12.04'
-                     '-amd64-server-20131205-disk1.img')
     saucy_image = ('ubuntu-released/ubuntu-saucy-13.10'
                    '-amd64-server-20140129-disk1.img')
     test_bed = TestBed('vila1', 'm1.small', saucy_image,

=== modified file 'test_runner/tstrun/tests/test_data_store.py'
--- test_runner/tstrun/tests/test_data_store.py	2014-02-06 11:23:26 +0000
+++ test_runner/tstrun/tests/test_data_store.py	2014-02-12 21:16:08 +0000
@@ -14,7 +14,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import errno
-import os
 import unittest
 
 

=== modified file 'tests/data_store/test.py'
--- tests/data_store/test.py	2014-02-10 19:11:11 +0000
+++ tests/data_store/test.py	2014-02-12 21:16:08 +0000
@@ -20,7 +20,8 @@
 import sys
 import yaml
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'ci-utils'))
+sys.path.append(os.path.join(
+    os.path.dirname(__file__), '..', '..', 'ci-utils'))
 
 from ci_utils import data_store
 
@@ -106,15 +107,14 @@
 
         del self.auth_config['auth_region']
         with self.assertRaises(data_store.DataStoreException):
-            ds = data_store.DataStore("test2", auth_config=self.auth_config)
+            data_store.DataStore("test2", auth_config=self.auth_config)
 
     def test_bad_region(self):
         """ Test that passing an invalid region fails. """
 
         self.auth_config['auth_region'] = 'bad_region'
         with self.assertRaises(data_store.DataStoreException):
-            ds = data_store.DataStore("test2",
-                                           auth_config=self.auth_config)
+            data_store.DataStore("test2", auth_config=self.auth_config)
 
 
 if __name__ == "__main__":