Code review comment for lp:~allenap/maas/remove-squashfs

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

On 13 November 2012 17:11, Andres Rodriguez <...> wrote:
...
>> For the size and benefit of the feature - i.e. a ~1 minute speedier
>> install - it's attracting a disproportionately high number of
>> bugs. That's my impression - I haven't tried to quantify it - but I'm
>> not alone in thinking this.
>
> 2 minute to be exact.

It's still an optimisation; it's neither needed for correct operation,
nor is it such a huge improvement that we could call it a killer
feature. FPI may qualify as the latter.

>
>
>> What is broken is the lack of tests. "Untested code is broken code" is
>> a principle that the Launchpad teams have all tried to follow for
>> years. It would be good to reintroduce this feature later on, or FPI,
>> but not in its current form.
>
> Right, but I was just pointing out that it isn't the only thing
> untested that maas ships, is it? However, its been placed as a
> strong reason for dropping the support.

Most parts that ship are tested; if something isn't tested it's a bug.

Notably the maas-import-* scripts are almost untested (there are basic
tests in provisioningserver.tests.test_maas_import_pxe_files, though
these just check the "happy path"; they don't demonstrate what happens
when there's an error).

I'd like to either massively increase test coverage for these scripts,
or remove them.

I'm not sure we have the time to improve test coverage, given that
writing tests often involves refactoring to make code testable, and
that we have no baseline to refactor against except for manual QA...
because there are almost no existing tests.

We need maas-import-pxe-files, -ephemerals and -isos, so we can't
remove them. However, we don't *need* squashfs support, so we don't
need maas-import-squashfs. Given that the feature does not work, and
that it needs non-trivial investment to get it working and covered by
tests, there's really no good argument for keeping it in trunk right
now.

« Back to merge proposal