Merge lp:~allenap/maas/fix-makefile-write-dhcp-stuff into lp:~maas-committers/maas/trunk

[Gavin Panella]

Tweak the way the DHCPConfigWriter is plumbed in.

This changes a few bits about the new DHCPConfigWriter.

- Renames write_dhcp_config.py to writer.py. It's in the
  provisioningserver.dhcp module, and it hardly needs explaining that
  it's about configuration.

- Removes the write_dhcp_config script entirely. It's not necessary -
  use bin/py -m provisioningserver/dhcp/writer instead - and is
  misleading because packages cannot use it anyway as it is generated
  by buildout.

- Remove all references to write_dhcp_config from Makefile.

- Remove the test for the write_dhcp_config script.

- Use stdout.write() to emit the config instead of print(); the latter
  adds an extra newline.

- Encodes the config as ASCII for the paranoid.

- Drive-by: restricts the scripts that the [database] part in
  buildout.cfg creates to just the bin/database script. Previously it
  was also generating a bin/postgresfixture script.

[Raphaël Badin (rvb)]

Looks good. +15/-66 without loosing functionality, that's nice.

Maybe we can wait for Julian to clarify things about [0] but I'll leave it up to you because, in my opinion, this is all good.

[0]

> Removes the write_dhcp_config script entirely. It's not necessary -
> use bin/py -m provisioningserver/dhcp/writer instead - and is
> misleading because packages cannot use it anyway as it is generated
> by buildout.

I agree with you. Since Julian is away I can only speculate here but this script can only be a development convenience and if it's the case then using the command as you suggest is, I think, good enough.

So far, when we had the need to expose a command, the usual way to do that has been to create a Django command. Now, unless I'm mistaken, this is part of the provisioning server because it will be run by the workers. We haven't talked about this but the workers will probably need to install a specific maas package (something like 'maas-worker') rather that the server 'maas' package and this worker package will only contain the worker code (i.e. the module 'provisioningserver'). This implies that if we want to ship scripts in this worker package we will have to come up with a new strategy because the Django MAAS application (and thus CLI) won't be installed there.

review: Approve

[Gavin Panella (allenap)]

If these scripts are only needed by packaging then we can just use the
python -m ... forms. For users, it may be worth having a single
front-end script, e.g. maas-provision or maas-worker, that follows the
verb pattern, or subparsers as argparse calls it. In other words,
something like:

  maas-provision verb --verb-specific-option

Or, more specifically:

  maas-provision dhcp-config --subnet ...

[Julian Edwards (julian-edwards)]

On Friday 08 Jun 2012 12:49:33 you wrote:
> Review: Approve
>
> Looks good. +15/-66 without loosing functionality, that's nice.

'losing'

> Maybe we can wait for Julian to clarify things about [0] but I'll leave it
> up to you because, in my opinion, this is all good.
>
> [0]
>
> > Removes the write_dhcp_config script entirely. It's not necessary -
> >
> > use bin/py -m provisioningserver/dhcp/writer instead - and is
> > misleading because packages cannot use it anyway as it is generated
> > by buildout.
>
> I agree with you. Since Julian is away I can only speculate here but this
> script can only be a development convenience and if it's the case then
> using the command as you suggest is, I think, good enough.

I'm kinda unhappy about this change, I'd have at least expected you to talk to
me before just removing my work. I put the generated script in as a
development convenience, because the bin/py example above is pretty hideous to
remember and type. I intended the package to use the module's script as-is
however, because it will have set up paths already.

Also, the branch has removed a test which checks the stdout output, and the
new name "writer" is inaccurate. If you want to split hairs, it's
"write_config".

I'd appreciate it if you'd put the script back, either buildout-generated or a
shortcut using the bin/py method.

Why did you remove the build dependency from the check target? Regardless of
my changes it seems a good idea.

[Gavin Panella (allenap)]

I guess I probably ought not to have done this branch... I was just
itching to do something and I didn't have the presence of mind to back
off. Anyway, point by point:

> I'm kinda unhappy about this change, I'd have at least expected you
> to talk to me before just removing my work.

I figured you were away for the weekend now and probably unavailable,
it's easy to undo stuff, and I am hoping to seek forgiveness instead
of getting advance permission.

> I put the generated script in as a development convenience, because
> the bin/py example above is pretty hideous to remember and type. I
> intended the package to use the module's script as-is however,
> because it will have set up paths already.

The extra section in buildout.cfg wasn't needed for example, though
that's not much of an issue. Really I felt that this was the wrong way
to go about exposing commands. The conveniences we create for
ourselves as developers will inevitably flow downstream, and I would
like to avoid that in this case. The `python -m module` pattern
prevents PATH pollution and also keeps us aware that exposing
provisioning commands to users is not a well solved problem.

The Django side of MAAS has management commands. I think the
provisioning side should have something similar, a "command verb"
pattern, like django-admin or bzr for example.

Also, if we want to expose non-developer scripts, we ought to put them
in setup.py instead of buildout.cfg. I think this'll make them easier
for packagers to use.

> Also, the branch has removed a test which checks the stdout output,

I didn't realise that was what it was trying to test. I'll add a test
for this, but I'll use self.patch(sys, "stdout", ...) instead, if
that's okay?

> and the new name "writer" is inaccurate. If you want to split hairs,
> it's "write_config".

Every other package and module name, afaict, is a noun. Another
natural place for DHCPConfigWriter could be
provisioninserver.dhcp.scripts?

> I'd appreciate it if you'd put the script back, either
> buildout-generated or a shortcut using the bin/py method.

Okay.

> Why did you remove the build dependency from the check target?
> Regardless of my changes it seems a good idea.

Yeah, it is a good idea. This was actually the thing that got me
started on this branch: the massive rubber hammer to squash a single
missing dependency.

So, given that testing a full build is good, but that it's worthwhile
exercising the dependencies in Makefile too, I'll change it to do a
full build *after* running tests.

=== modified file 'Makefile'
--- Makefile	2012-06-08 00:23:22 +0000
+++ Makefile	2012-06-08 10:31:19 +0000
@@ -21,7 +21,6 @@
     bin/twistd.pserv bin/test.pserv \
     bin/twistd.txlongpoll \
     bin/py bin/ipy \
-    bin/write_dhcp_config \
     $(js_enums)
 
 all: build doc
@@ -70,10 +69,6 @@
 	bin/buildout install repl
 	@touch --no-create bin/py bin/ipy
 
-bin/write_dhcp_config: bin/buildout buildout.cfg versions.cfg setup.py
-	bin/buildout install write-dhcp-config
-	@touch --no-create $@
-
 test: bin/test.maas bin/test.maastesting bin/test.pserv $(js_enums)
 	bin/test.maas
 	bin/test.maastesting
@@ -95,7 +90,7 @@
 lint-js:
 	@find $(sources) -type f -print0 | xargs -r0 $(pocketlint)
 
-check: clean build test
+check: clean test
 
 docs/api.rst: bin/maas src/maasserver/api.py syncdb
 	bin/maas generate_api_doc > $@

=== modified file 'buildout.cfg'
--- buildout.cfg	2012-06-07 05:54:29 +0000
+++ buildout.cfg	2012-06-08 10:31:19 +0000
@@ -9,7 +9,6 @@
   repl
   sphinx
   txlongpoll
-  write-dhcp-config
 extensions = buildout-versions
 buildout_versions_file = versions.cfg
 versions = versions
@@ -80,6 +79,7 @@
 extra-paths = ${common:extra-paths}
 interpreter =
 entry-points = database=postgresfixture.main:main
+scripts = database
 
 [maas]
 recipe = zc.recipe.egg
@@ -214,9 +214,3 @@
   txamqp
 entry-points = twistd.txlongpoll=twisted.scripts.twistd:run
 scripts = twistd.txlongpoll
-
-[write-dhcp-config]
-recipe = z3c.recipe.scripts
-extra-paths = ${common:extra-paths}
-entry-points = write_dhcp_config=provisioningserver.dhcp.write_dhcp_config:run
-scripts = write_dhcp_config

=== renamed file 'src/provisioningserver/dhcp/tests/test_write_dhcp_config.py' => 'src/provisioningserver/dhcp/tests/test_writer.py'
--- src/provisioningserver/dhcp/tests/test_write_dhcp_config.py	2012-06-07 07:53:25 +0000
+++ src/provisioningserver/dhcp/tests/test_writer.py	2012-06-08 10:31:19 +0000
@@ -1,7 +1,7 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for write-dhcp-config.py"""
+"""Tests for `provisioningserver.dhcp.writer`."""
 
 from __future__ import (
     absolute_import,
@@ -13,19 +13,15 @@
 __all__ = []
 
 import os
-import subprocess
 
 from maastesting.matchers import ContainsAll
 from maastesting.testcase import TestCase
-from testtools.matchers import (
-    MatchesStructure,
-    )
-
-from provisioningserver.dhcp.write_dhcp_config import DHCPConfigWriter
-
-
-class TestModule(TestCase):
-    """Test the write-dhcp-config module."""
+from provisioningserver.dhcp.writer import DHCPConfigWriter
+from testtools.matchers import MatchesStructure
+
+
+class TestDHCPConfigWriter(TestCase):
+    """Test `DHCPConfigWriter`."""
 
     def test_arg_setup(self):
         writer = DHCPConfigWriter()
@@ -92,32 +88,3 @@
         writer.run(test_args)
 
         self.assertTrue(os.path.exists(outfile))
-
-
-class TestScriptExecutable(TestCase):
-    """Test that the actual script is executable."""
-
-    def test_script(self):
-        test_args = [
-            '--subnet', 'subnet',
-            '--subnet-mask', 'subnet-mask',
-            '--next-server', 'next-server',
-            '--broadcast-address', 'broadcast-address',
-            '--dns-servers', 'dns-servers',
-            '--gateway', 'gateway',
-            '--low-range', 'low-range',
-            '--high-range', 'high-range',
-            ]
-
-        exe = [os.path.join(
-            os.path.dirname(__file__),
-            os.pardir, os.pardir, os.pardir, os.pardir,
-            "bin", "write_dhcp_config")]
-
-        exe.extend(test_args)
-        output = subprocess.check_output(exe)
-
-        contains_all_params = ContainsAll(
-            ['subnet', 'subnet-mask', 'next-server', 'broadcast-address',
-            'dns-servers', 'gateway', 'low-range', 'high-range'])
-        self.assertThat(output, contains_all_params)

=== renamed file 'src/provisioningserver/dhcp/write_dhcp_config.py' => 'src/provisioningserver/dhcp/writer.py'
--- src/provisioningserver/dhcp/write_dhcp_config.py	2012-06-07 07:53:25 +0000
+++ src/provisioningserver/dhcp/writer.py	2012-06-08 10:31:19 +0000
@@ -12,8 +12,8 @@
 __metaclass__ = type
 __all__ = []
 
-
 import argparse
+from sys import stdout
 
 from provisioningserver.dhcp import config
 
@@ -73,20 +73,13 @@
     def run(self, argv=None):
         """Generate the config and write to stdout or a file as required."""
         args = self.parse_args(argv)
-        output = self.generate(args)
-        outfile = args.out_file
-        if outfile is not None:
-            with open(outfile, "w") as f:
+        output = self.generate(args).encode("ascii")
+        if args.out_file is not None:
+            with open(args.out_file, "wb") as f:
                 f.write(output)
         else:
-            print(output)
-
-
-def run():
-    """Entry point for scripts."""
-    writer = DHCPConfigWriter()
-    writer.run()
+            stdout.write(output)
 
 
 if __name__ == "__main__":
-    run()
+    DHCPConfigWriter().run()