Code review comment for lp:~allenap/maas/fix-makefile-write-dhcp-stuff

Revision history for this message
Gavin Panella (allenap) wrote :

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.

« Back to merge proposal