Merge into trunk : create-hwpack-skeleton : Code : Linaro Image Tools

Status:	Merged
Merged at revision:	56
Proposed branch:	lp://qastaging/~james-w/linaro-image-tools/create-hwpack-skeleton
Merge into:	lp://qastaging/linaro-image-tools/11.11
Prerequisite:	lp://qastaging/~james-w/linaro-image-tools/tarfile-matchers
Diff against target:	632 lines (+391/-84) 5 files modified hwpack/config.py (+26/-20) hwpack/hardwarepack.py (+120/-0) hwpack/tests/__init__.py (+1/-0) hwpack/tests/test_config.py (+56/-64) hwpack/tests/test_hardwarepack.py (+188/-0)
To merge this branch:	bzr merge lp://qastaging/~james-w/linaro-image-tools/create-hwpack-skeleton
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Guilherme Salgado (community)		2010-08-31	Approve on 2010-09-02
Review via email: mp+34233@code.qastaging.launchpad.net

Description of the change

Hi,

Here's the skeleton of hwpack creation. It's a couple of classes that you can
instantiate and have them co-operate to produce a perfectly valid, if useless,
hardware pack.

I wanted to push this up, and then I'll followup with methods to actually add
packages etc. once I know what API we want for that.

Thanks,

James

lp://qastaging/~james-w/linaro-image-tools/create-hwpack-skeleton updated on 2010-09-01

73. By James Westby on 2010-08-31: Add docstrings.
74. By James Westby on 2010-08-31: Merged config-changes into create-hwpack-skeleton.
75. By James Westby on 2010-08-31: Merged config-changes into create-hwpack-skeleton.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-09-01:

#

Download full text (10.8 KiB)

Hi James,

I have a few suggestions, but this looks very good.

review approve

On Tue, 2010-08-31 at 18:44 +0000, James Westby wrote:
> James Westby has proposed merging lp:~james-w/linaro-image-tools/create-hwpack-skeleton into lp:linaro-image-tools with lp:~james-w/linaro-image-tools/tarfile-matchers as a prerequisite.
>
> Requested reviews:
> Linaro Infrastructure (linaro-infrastructure)
>
>
> Hi,
>
> Here's the skeleton of hwpack creation. It's a couple of classes that you can
> instantiate and have them co-operate to produce a perfectly valid, if useless,
> hardware pack.
>
> I wanted to push this up, and then I'll followup with methods to actually add
> packages etc. once I know what API we want for that.
>
> Thanks,
>
> James
> differences between files attachment (review-diff.txt)
> === added file 'hwpack/hardwarepack.py'
> --- hwpack/hardwarepack.py 1970-01-01 00:00:00 +0000
> +++ hwpack/hardwarepack.py 2010-08-31 18:44:46 +0000
> @@ -0,0 +1,63 @@
> +from hwpack.better_tarfile import writeable_tarfile
> +
> +
> +class Metadata(object):
> +
> + def __init__(self, name, version, origin=None, maintainer=None,
> + support=None):
> + self.name = name
> + self.version = version
> + self.origin = origin
> + self.maintainer = maintainer
> + self.support = support
> +
> + def __str__(self):
> + metadata = "NAME=%s\n" % self.name
> + metadata += "VERSION=%s\n" % self.version
> + if self.origin is not None:
> + metadata += "ORIGIN=%s\n" % self.origin
> + if self.maintainer is not None:
> + metadata += "MAINTAINER=%s\n" % self.maintainer
> + if self.support is not None:
> + metadata += "SUPPORT=%s\n" % self.support
> + return metadata
> +
> +
> +class HardwarePack(object):
> +
> + FORMAT = "1.0"
> + FORMAT_FILENAME = "FORMAT"
> + METADATA_FILENAME = "metadata"
> + MANIFEST_FILENAME = "manifest"
> + PACKAGES_DIRNAME = "pkgs"
> + PACKAGES_FILENAME = "%s/Packages" % PACKAGES_DIRNAME
> + SOURCES_LIST_DIRNAME = "sources.list.d"
> + SOURCES_LIST_GPG_DIRNAME = "sources.list.d.gpg"
> +
> + def __init__(self, metadata):
> + self.metadata = metadata
> +
> + def filename(self):
> + if self.metadata.support is None:
> + support_suffix = ""
> + else:
> + support_suffix = "_%s" % self.metadata.support
> + return "hwpack_%s_%s%s.tar.gz" % (
> + self.metadata.name, self.metadata.version, support_suffix)
> +
> + def to_f(self, fileobj):

Why not to_file()?

> + kwargs = {}
> + kwargs["default_uid"] = 1000
> + kwargs["default_gid"] = 1000
> + kwargs["default_uname"] = "user"
> + kwargs["default_gname"] = "group"
> + with writeable_tarfile(fileobj, mode="w:gz", **kwargs) as tf:
> + tf.create_file_from_string(
> + self.FORMAT_FILENAME, self.FORMAT + "\n")
> + tf.create_file_from_string(
> + self.METADATA_FILENAME, str(self.metadata))
> + tf.create_file_from_string(self.MANIFEST_FILENAME, "")
> + tf.create_dir(self.PAC...

Hi James,

I have a few suggestions, but this looks very good.

review approve

On Tue, 2010-08-31 at 18:44 +0000, James Westby wrote:
> James Westby has proposed merging lp:~james-w/linaro-image-tools/create-hwpack-skeleton into lp:linaro-image-tools with lp:~james-w/linaro-image-tools/tarfile-matchers as a prerequisite.
> 
> Requested reviews:
>   Linaro Infrastructure (linaro-infrastructure)
> 
> 
> Hi,
> 
> Here's the skeleton of hwpack creation. It's a couple of classes that you can
> instantiate and have them co-operate to produce a perfectly valid, if useless,
> hardware pack.
> 
> I wanted to push this up, and then I'll followup with methods to actually add
> packages etc. once I know what API we want for that.
> 
> Thanks,
> 
> James
> differences between files attachment (review-diff.txt)
> === added file 'hwpack/hardwarepack.py'
> --- hwpack/hardwarepack.py	1970-01-01 00:00:00 +0000
> +++ hwpack/hardwarepack.py	2010-08-31 18:44:46 +0000
> @@ -0,0 +1,63 @@
> +from hwpack.better_tarfile import writeable_tarfile
> +
> +
> +class Metadata(object):
> +
> +    def __init__(self, name, version, origin=None, maintainer=None,
> +                 support=None):
> +        self.name = name
> +        self.version = version
> +        self.origin = origin
> +        self.maintainer = maintainer
> +        self.support = support
> +
> +    def __str__(self):
> +        metadata = "NAME=%s\n" % self.name
> +        metadata += "VERSION=%s\n" % self.version
> +        if self.origin is not None:
> +            metadata += "ORIGIN=%s\n" % self.origin
> +        if self.maintainer is not None:
> +            metadata += "MAINTAINER=%s\n" % self.maintainer
> +        if self.support is not None:
> +            metadata += "SUPPORT=%s\n" % self.support
> +        return metadata
> +
> +
> +class HardwarePack(object):
> +
> +    FORMAT = "1.0"
> +    FORMAT_FILENAME = "FORMAT"
> +    METADATA_FILENAME = "metadata"
> +    MANIFEST_FILENAME = "manifest"
> +    PACKAGES_DIRNAME = "pkgs"
> +    PACKAGES_FILENAME = "%s/Packages" % PACKAGES_DIRNAME
> +    SOURCES_LIST_DIRNAME = "sources.list.d"
> +    SOURCES_LIST_GPG_DIRNAME = "sources.list.d.gpg"
> +
> +    def __init__(self, metadata):
> +        self.metadata = metadata
> +
> +    def filename(self):
> +        if self.metadata.support is None:
> +            support_suffix = ""
> +        else:
> +            support_suffix = "_%s" % self.metadata.support
> +        return "hwpack_%s_%s%s.tar.gz" % (
> +            self.metadata.name, self.metadata.version, support_suffix)
> +
> +    def to_f(self, fileobj):

Why not to_file()?

> +        kwargs = {}
> +        kwargs["default_uid"] = 1000
> +        kwargs["default_gid"] = 1000
> +        kwargs["default_uname"] = "user"
> +        kwargs["default_gname"] = "group"
> +        with writeable_tarfile(fileobj, mode="w:gz", **kwargs) as tf:
> +            tf.create_file_from_string(
> +                self.FORMAT_FILENAME, self.FORMAT + "\n")
> +            tf.create_file_from_string(
> +                self.METADATA_FILENAME, str(self.metadata))
> +            tf.create_file_from_string(self.MANIFEST_FILENAME, "")
> +            tf.create_dir(self.PACKAGES_DIRNAME)
> +            tf.create_file_from_string(self.PACKAGES_FILENAME, "")
> +            tf.create_dir(self.SOURCES_LIST_DIRNAME)
> +            tf.create_dir(self.SOURCES_LIST_GPG_DIRNAME)

I assume subsequent branches will change the code above to set the content
for the directories/files created here (like Metadata)?  Might be worth adding
a TODO comment to that effect.

> 
> === added file 'hwpack/tests/test_hardwarepack.py'
> --- hwpack/tests/test_hardwarepack.py	1970-01-01 00:00:00 +0000
> +++ hwpack/tests/test_hardwarepack.py	2010-08-31 18:44:46 +0000
> @@ -0,0 +1,155 @@
> +from StringIO import StringIO
> +import tarfile
> +
> +from testtools import TestCase
> +
> +from hwpack.hardwarepack import HardwarePack, Metadata
> +from hwpack.tarfile_matchers import TarfileHasFile
> +
> +
> +class MetadataTests(TestCase):
> +
> +    def test_name(self):
> +        metadata = Metadata("ahwpack", "3")
> +        self.assertEqual("ahwpack", metadata.name)
> +
> +    def test_version(self):
> +        metadata = Metadata("ahwpack", "3")
> +        self.assertEqual("3", metadata.version)
> +
> +    def test_default_origin_is_None(self):
> +        metadata = Metadata("ahwpack", "4")
> +        self.assertEqual(None, metadata.origin)
> +
> +    def test_origin(self):
> +        metadata = Metadata("ahwpack", "4", origin="linaro")
> +        self.assertEqual("linaro", metadata.origin)
> +
> +    def test_default_maintainer_is_None(self):
> +        metadata = Metadata("ahwpack", "4")
> +        self.assertEqual(None, metadata.maintainer)
> +
> +    def test_maintainer(self):
> +        metadata = Metadata("ahwpack", "4", maintainer="Some maintainer")
> +        self.assertEqual("Some maintainer", metadata.maintainer)
> +
> +    def test_default_support_is_None(self):
> +        metadata = Metadata("ahwpack", "4")
> +        self.assertEqual(None, metadata.support)
> +
> +    def test_support(self):
> +        metadata = Metadata("ahwpack", "4", support="supported")
> +        self.assertEqual("supported", metadata.support)

Do you think it's really worth having all those nearly identical test methods
for each of the arguments of __init__?  I'd have written just a couple test
methods for it instead: one to show the default values and one to show that
the instance variables are set when specified.  Maybe also one to show which
arguments are required.  I don't feel strong about it, though, so I'm happy
with whatever you prefer.

> +
> +    def test_str(self):
> +        metadata = Metadata("ahwpack", "4")
> +        self.assertEqual("NAME=ahwpack\nVERSION=4\n", str(metadata))
> +
> +    def test_str_with_origin(self):
> +        metadata = Metadata("ahwpack", "4", origin="linaro")
> +        self.assertEqual(
> +            "NAME=ahwpack\nVERSION=4\nORIGIN=linaro\n", str(metadata))
> +
> +    def test_str_with_maintainer(self):
> +        metadata = Metadata("ahwpack", "4", maintainer="Some Maintainer")
> +        self.assertEqual(
> +            "NAME=ahwpack\nVERSION=4\nMAINTAINER=Some Maintainer\n",
> +            str(metadata))
> +
> +    def test_str_with_support(self):
> +        metadata = Metadata("ahwpack", "4", support="unsupported")
> +        self.assertEqual(
> +            "NAME=ahwpack\nVERSION=4\nSUPPORT=unsupported\n", str(metadata))
> +
> +
> +class HardwarePackTests(TestCase):
> +
> +    def setUp(self):
> +        super(HardwarePackTests, self).setUp()
> +        self.metadata = Metadata("ahwpack", 4)
> +
> +    def test_format_is_1_0(self):
> +        hwpack = HardwarePack(self.metadata)
> +        self.assertEqual("1.0", hwpack.FORMAT)
> +
> +    def test_filename(self):
> +        hwpack = HardwarePack(self.metadata)
> +        self.assertEqual("hwpack_ahwpack_4.tar.gz", hwpack.filename())
> +
> +    def test_filename_with_support(self):
> +        metadata = Metadata("ahwpack", "4", support="supported")
> +        hwpack = HardwarePack(metadata)
> +        self.assertEqual(
> +            "hwpack_ahwpack_4_supported.tar.gz", hwpack.filename())
> +
> +    def get_tarfile(self, hwpack):
> +        fileobj = StringIO()
> +        hwpack.to_f(fileobj)
> +        fileobj.seek(0)
> +        tf = tarfile.open(mode="r:gz", fileobj=fileobj)
> +        self.addCleanup(tf.close)
> +        return tf
> +
> +    def assertHasPath(self, tarball, path, **kwargs):

It's not clear to me what this assertion method does without looking at the
implementation of TarfileHasFile.  Is it asserting the file's content as well
or just that the path exists in the tarball?  I think it'd be worthwhile to
have that in a docstring.

> +        kwargs.setdefault("type", tarfile.REGTYPE)
> +        if "content" in kwargs:
> +            kwargs.setdefault("size", len(kwargs["content"]))
> +        if kwargs["type"] == tarfile.DIRTYPE:
> +            kwargs.setdefault("mode", 0755)
> +        else:
> +            kwargs.setdefault("mode", 0644)
> +        kwargs.setdefault("linkname", "")
> +        kwargs.setdefault("uid", 1000)
> +        kwargs.setdefault("gid", 1000)
> +        kwargs.setdefault("uname", "user")
> +        kwargs.setdefault("gname", "group")
> +        # TODO: mtime checking
> +        self.assertThat(tarball, TarfileHasFile(path, **kwargs))
> +
> +    def test_creates_FORMAT_file(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "FORMAT", content=hwpack.FORMAT+"\n")
> +
> +    def test_creates_metadata_file(self):
> +        metadata = Metadata(
> +            "ahwpack", "4", origin="linaro",
> +            maintainer="Some Maintainer", support="unsupported")
> +        hwpack = HardwarePack(metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "metadata", content=str(metadata))
> +
> +    def test_creates_manifest_file(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "manifest")
> +
> +    def test_manifest_file_empty_with_no_packages(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "manifest", content="")
> +
> +    def test_creates_pkgs_dir(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "pkgs", type=tarfile.DIRTYPE)
> +
> +    def test_creates_Packages_file(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "pkgs/Packages")
> +
> +    def test_Packages_file_empty_with_no_packages(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "pkgs/Packages", content="")
> +
> +    def test_creates_sources_list_dir(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "sources.list.d", type=tarfile.DIRTYPE)
> +
> +    def test_creates_sources_list_gpg_dir(self):
> +        hwpack = HardwarePack(self.metadata)
> +        tf = self.get_tarfile(hwpack)
> +        self.assertHasPath(tf, "sources.list.d.gpg", type=tarfile.DIRTYPE)

Now that I've seen how assertHasPath() is used, it becomes clear what it's
doing but I'm wondering if we do should use **kwargs there -- I think
I wouldn't need a docstring to understand what it does if the keyword
arguments were explicitly listed there.  Using **kwargs simplify things
slightly as we don't need to worry about keeping the list of keyword
arguments in sync, but here we seem to need only the content and type
arguments, so it might be better to have just those in assertHasPath(). What
do you think?
-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-- 
Guilherme Salgado <https://launchpad.net/~salgado>

review: Approve

Revision history for this message

James Westby (james-w) wrote on 2010-09-01:

#

Download full text (3.4 KiB)

> Hi James,
>
> I have a few suggestions, but this looks very good.

Thanks for the review.

> > + def to_f(self, fileobj):
>
> Why not to_file()?

I can change it. I thought that to_f() was less ambiguous about whether it took
a path or a filehandle, but to_f() is just generally ambiguous :-)

> > + kwargs = {}
> > + kwargs["default_uid"] = 1000
> > + kwargs["default_gid"] = 1000
> > + kwargs["default_uname"] = "user"
> > + kwargs["default_gname"] = "group"
> > + with writeable_tarfile(fileobj, mode="w:gz", **kwargs) as tf:
> > + tf.create_file_from_string(
> > + self.FORMAT_FILENAME, self.FORMAT + "\n")
> > + tf.create_file_from_string(
> > + self.METADATA_FILENAME, str(self.metadata))
> > + tf.create_file_from_string(self.MANIFEST_FILENAME, "")
> > + tf.create_dir(self.PACKAGES_DIRNAME)
> > + tf.create_file_from_string(self.PACKAGES_FILENAME, "")
> > + tf.create_dir(self.SOURCES_LIST_DIRNAME)
> > + tf.create_dir(self.SOURCES_LIST_GPG_DIRNAME)
>
> I assume subsequent branches will change the code above to set the content
> for the directories/files created here (like Metadata)? Might be worth adding
> a TODO comment to that effect.

Yes, this is completely useless now, as while it produces a valid hwpack,
it doesn't contain any sources or packages. I wanted to explore how
we would fetch packages before adding an interface to put them in the
hwpack (do we want to take strings, filehandles, or uris? One at a time,
or bulk insertion)

I can add the TODO.

> Do you think it's really worth having all those nearly identical test methods
> for each of the arguments of __init__? I'd have written just a couple test
> methods for it instead: one to show the default values and one to show that
> the instance variables are set when specified. Maybe also one to show which
> arguments are required. I don't feel strong about it, though, so I'm happy
> with whatever you prefer.

Yeah, it's repetitive, but I was just following Michael's advice that the
best test methods have only two lines. I'll leave them as they are for now,
but will collapse them if it gets tedious.

> > + def assertHasPath(self, tarball, path, **kwargs):
>
> It's not clear to me what this assertion method does without looking at the
> implementation of TarfileHasFile. Is it asserting the file's content as well
> or just that the path exists in the tarball? I think it'd be worthwhile to
> have that in a docstring.

> Now that I've seen how assertHasPath() is used, it becomes clear what it's
> doing but I'm wondering if we do should use **kwargs there -- I think
> I wouldn't need a docstring to understand what it does if the keyword
> arguments were explicitly listed there. Using **kwargs simplify things
> slightly as we don't need to worry about keeping the list of keyword
> arguments in sync, but here we seem to need only the content and type
> arguments, so it might be better to have just those in assertHasPath(). What
> do you think?

The reason that I added a method was so that we could have default checks
for mtime, uid...

> Hi James,
> 
> I have a few suggestions, but this looks very good.

Thanks for the review.

> > +    def to_f(self, fileobj):
> 
> Why not to_file()?

I can change it. I thought that to_f() was less ambiguous about whether it took
a path or a filehandle, but to_f() is just generally ambiguous :-)

> > +        kwargs = {}
> > +        kwargs["default_uid"] = 1000
> > +        kwargs["default_gid"] = 1000
> > +        kwargs["default_uname"] = "user"
> > +        kwargs["default_gname"] = "group"
> > +        with writeable_tarfile(fileobj, mode="w:gz", **kwargs) as tf:
> > +            tf.create_file_from_string(
> > +                self.FORMAT_FILENAME, self.FORMAT + "\n")
> > +            tf.create_file_from_string(
> > +                self.METADATA_FILENAME, str(self.metadata))
> > +            tf.create_file_from_string(self.MANIFEST_FILENAME, "")
> > +            tf.create_dir(self.PACKAGES_DIRNAME)
> > +            tf.create_file_from_string(self.PACKAGES_FILENAME, "")
> > +            tf.create_dir(self.SOURCES_LIST_DIRNAME)
> > +            tf.create_dir(self.SOURCES_LIST_GPG_DIRNAME)
> 
> I assume subsequent branches will change the code above to set the content
> for the directories/files created here (like Metadata)?  Might be worth adding
> a TODO comment to that effect.

Yes, this is completely useless now, as while it produces a valid hwpack,
it doesn't contain any sources or packages. I wanted to explore how
we would fetch packages before adding an interface to put them in the
hwpack (do we want to take strings, filehandles, or uris? One at a time,
or bulk insertion)

I can add the TODO.

> Do you think it's really worth having all those nearly identical test methods
> for each of the arguments of __init__?  I'd have written just a couple test
> methods for it instead: one to show the default values and one to show that
> the instance variables are set when specified.  Maybe also one to show which
> arguments are required.  I don't feel strong about it, though, so I'm happy
> with whatever you prefer.

Yeah, it's repetitive, but I was just following Michael's advice that the
best test methods have only two lines. I'll leave them as they are for now,
but will collapse them if it gets tedious.

> > +    def assertHasPath(self, tarball, path, **kwargs):
> 
> It's not clear to me what this assertion method does without looking at the
> implementation of TarfileHasFile.  Is it asserting the file's content as well
> or just that the path exists in the tarball?  I think it'd be worthwhile to
> have that in a docstring.

> Now that I've seen how assertHasPath() is used, it becomes clear what it's
> doing but I'm wondering if we do should use **kwargs there -- I think
> I wouldn't need a docstring to understand what it does if the keyword
> arguments were explicitly listed there.  Using **kwargs simplify things
> slightly as we don't need to worry about keeping the list of keyword
> arguments in sync, but here we seem to need only the content and type
> arguments, so it might be better to have just those in assertHasPath(). What
> do you think?

The reason that I added a method was so that we could have default checks
for mtime, uid, etc., so that all paths had those checks done on them, rather
than lots of tests.

Would it be better if I made a subclass of TarfileHasFile that put the
defaults in? Would you want it to document the arguments, or use **kwargs
and point to the superclass, explaining what the defaults are it uses?

Thanks,

James

Revision history for this message

James Westby (james-w) wrote on 2010-09-02:

#

> The reason that I added a method was so that we could have default checks
> for mtime, uid, etc., so that all paths had those checks done on them, rather
> than lots of tests.
>
> Would it be better if I made a subclass of TarfileHasFile that put the
> defaults in? Would you want it to document the arguments, or use **kwargs
> and point to the superclass, explaining what the defaults are it uses?

Done, let me know what you think.

Thanks,

James

lp://qastaging/~james-w/linaro-image-tools/create-hwpack-skeleton updated on 2010-09-02

76. By James Westby on 2010-09-01: Rename to_f to to_file, thanks Guilherme.
77. By James Westby on 2010-09-01: Add a TODO, thanks Guilherme.
78. By James Westby on 2010-09-02: Use a subclass of the matcher, rather than a custom assertion method.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2010-09-02:

#

Download full text (3.9 KiB)

On Wed, 2010-09-01 at 21:53 +0000, James Westby wrote:
> > Hi James,
> >
> > I have a few suggestions, but this looks very good.
>
> Thanks for the review.
>
> > > + def to_f(self, fileobj):
> >
> > Why not to_file()?
>
> I can change it. I thought that to_f() was less ambiguous about whether it took
> a path or a filehandle, but to_f() is just generally ambiguous :-)

Cool, thanks. :)

>
> > > + kwargs = {}
> > > + kwargs["default_uid"] = 1000
> > > + kwargs["default_gid"] = 1000
> > > + kwargs["default_uname"] = "user"
> > > + kwargs["default_gname"] = "group"
> > > + with writeable_tarfile(fileobj, mode="w:gz", **kwargs) as tf:
> > > + tf.create_file_from_string(
> > > + self.FORMAT_FILENAME, self.FORMAT + "\n")
> > > + tf.create_file_from_string(
> > > + self.METADATA_FILENAME, str(self.metadata))
> > > + tf.create_file_from_string(self.MANIFEST_FILENAME, "")
> > > + tf.create_dir(self.PACKAGES_DIRNAME)
> > > + tf.create_file_from_string(self.PACKAGES_FILENAME, "")
> > > + tf.create_dir(self.SOURCES_LIST_DIRNAME)
> > > + tf.create_dir(self.SOURCES_LIST_GPG_DIRNAME)
> >
> > I assume subsequent branches will change the code above to set the content
> > for the directories/files created here (like Metadata)? Might be worth adding
> > a TODO comment to that effect.
>
> Yes, this is completely useless now, as while it produces a valid hwpack,
> it doesn't contain any sources or packages. I wanted to explore how
> we would fetch packages before adding an interface to put them in the
> hwpack (do we want to take strings, filehandles, or uris? One at a time,
> or bulk insertion)

That's fine, I just wanted to make sure this was the case.

>
> I can add the TODO.
>
> > Do you think it's really worth having all those nearly identical test methods
> > for each of the arguments of __init__? I'd have written just a couple test
> > methods for it instead: one to show the default values and one to show that
> > the instance variables are set when specified. Maybe also one to show which
> > arguments are required. I don't feel strong about it, though, so I'm happy
> > with whatever you prefer.
>
> Yeah, it's repetitive, but I was just following Michael's advice that the
> best test methods have only two lines. I'll leave them as they are for now,
> but will collapse them if it gets tedious.

Ok. I also try to keep test methods as simple as possible, but what
prompted me to ask about these ones is the fact that there are 8 tests
for some really trivial piece of code.

>
> > > + def assertHasPath(self, tarball, path, **kwargs):
> >
> > It's not clear to me what this assertion method does without looking at the
> > implementation of TarfileHasFile. Is it asserting the file's content as well
> > or just that the path exists in the tarball? I think it'd be worthwhile to
> > have that in a docstring.
>
> > Now that I've seen how assertHasPath() is used, it becomes clear what it's
> > doing but I'm wondering if we do should use **kwargs there -- I think
> > I wouldn't need a docstring to u...

Linaro Image Tools

Merge lp://qastaging/~james-w/linaro-image-tools/create-hwpack-skeleton into lp://qastaging/linaro-image-tools/11.11

Commit message

Description of the change

Preview Diff

Subscribers