Code review comment for lp:~clint-fewbar/review-new-branches/ceph-new-pkg

Revision history for this message
Mathias Gug (mathiaz) wrote :

* debian/changelog

  The file should be trimmed to only have one entry mentioning the
  initial packaging for Ubuntu and refering the need-packaging bug in LP
  (bug 607730):

    ceph (0.21-0ubuntu1) maverick; urgency=low

      * Initial release based on upstream package (LP: #607730).

* debian/control

  The maintainer field should be updated per
  https://wiki.ubuntu.com/DebianMaintainerField.

* debian/copyright

  I would recommend to format the content of this file according to
  the 'Machine-readable debian/copyright' DEP - see
  http://dep.debian.net/deps/dep5/.

  The Files: * section has an empty line on line 14.

  src/include/ceph_hash.cc: should also be mentioned as it's copyrighted
  under Public Domain.

  src/os/btrfs_ioctl.h: is under GPLv2.

  Running 'licensecheck -r .' in the top source directory will output a
  good estimate of the license and copyright for each file.

* debian/rules: I'd recommend to properly support autotools in the build
  system. See /usr/share/doc/autotools-dev/README.Debian.gz available
  from the autotools-dev package.

* Log files should be properly handled as outlined in the Debian Policy
  Section 10.8 [1]

  [1]: http://www.debian.org/doc/debian-policy/ch-files.html#s10.8

  Log directory should be deleted on package purge.

* Configuration files in /etc/ seem to be sample files given their name
  in ceph.install:

    etc/ceph/sample.ceph.conf
    etc/ceph/sample.fetch_config

  Are these files providing a default working configuration? If so their
  name should not start with sample. If not they should be shipped under
  /usr/share/doc/ceph/ and a default working configuration should be
  shipped as part of the package.

* AFAICT all commands to create a filesystem are located in /sbin/.
  mkcephfs should probably be there as well instead of
  usr/sbin/.

* debian/rados.install is not needed as there isn't any rados package defined
  in debian/control.

* Upon installation of the ceph package it seems that there isn't a
  default working configuration. Is it possible to provide a default
  working configuration?

review: Needs Fixing

« Back to merge proposal