launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/clean-up-locate-tftp-path into lp:maas with lp:~allenap/maas/install-dependencies-tweak as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/clean-up-locate-tftp-path/+merge/117293

An earlier branch of mine used an assertion to get round the fact that a keyword argument was now required: the tftproot arguments to locate_tftp_path. I've cleaned this up now by making all arguments to locate_tftp_path mandatory.


-- 
https://code.launchpad.net/~allenap/maas/clean-up-locate-tftp-path/+merge/117293
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/clean-up-locate-tftp-path into lp:maas.

=== modified file 'src/provisioningserver/pxe/tests/test_tftppath.py'
--- src/provisioningserver/pxe/tests/test_tftppath.py	2012-07-30 09:34:23 +0000
+++ src/provisioningserver/pxe/tests/test_tftppath.py	2012-07-30 16:28:21 +0000
@@ -87,19 +87,12 @@
             compose_bootloader_path(arch, subarch),
             Not(StartsWith(self.tftproot)))
 
-    def test_locate_tftp_path_prefixes_tftp_root_by_default(self):
+    def test_locate_tftp_path_prefixes_tftp_root(self):
         pxefile = factory.make_name('pxefile')
         self.assertEqual(
             os.path.join(self.tftproot, pxefile),
             locate_tftp_path(pxefile, tftproot=self.tftproot))
 
-    def test_locate_tftp_path_overrides_default_tftproot(self):
-        tftproot = '/%s' % factory.make_name('tftproot')
-        pxefile = factory.make_name('pxefile')
-        self.assertEqual(
-            os.path.join(tftproot, pxefile),
-            locate_tftp_path(pxefile, tftproot=tftproot))
-
-    def test_locate_tftp_path_returns_root_by_default(self):
-        self.assertEqual(
-            self.tftproot, locate_tftp_path(tftproot=self.tftproot))
+    def test_locate_tftp_path_returns_root_when_path_is_None(self):
+        self.assertEqual(
+            self.tftproot, locate_tftp_path(None, tftproot=self.tftproot))

=== modified file 'src/provisioningserver/pxe/tftppath.py'
--- src/provisioningserver/pxe/tftppath.py	2012-07-30 09:34:08 +0000
+++ src/provisioningserver/pxe/tftppath.py	2012-07-30 16:28:21 +0000
@@ -64,19 +64,17 @@
     return '/'.join(['/maas', arch, subarch, release, purpose])
 
 
-def locate_tftp_path(tftp_path=None, tftproot=None):
-    """Return the local filesystem path corresponding to `tftp_path`.
+def locate_tftp_path(path, tftproot):
+    """Return the local filesystem path corresponding to `path`.
 
     The return value gives the filesystem path where you'd have to put
-    a file if you wanted it made available over TFTP as `tftp_path`.
+    a file if you wanted it made available over TFTP as `path`.
 
-    :param tftp_path: Path as used in the TFTP protocol which you want
-        the local filesystem equivalent for. Omit this, or pass `None`,
-        to get the root of the TFTP hierarchy.
-    :param tftproot: Optional TFTP root directory to override the
-        configured default.
+    :param path: Path as used in the TFTP protocol which you want
+        the local filesystem equivalent for. Pass `None` to get the root
+        of the TFTP hierarchy.
+    :param tftproot: The TFTP root directory.
     """
-    assert tftproot is not None, "tftproot must be defined."
-    if tftp_path is None:
+    if path is None:
         return tftproot
-    return os.path.join(tftproot, tftp_path.lstrip('/'))
+    return os.path.join(tftproot, path.lstrip('/'))