add unit test for ValidateLayout function #4

xiekeyang · 2016-09-16T00:42:33Z

migrate patch from opencontainers/image-spec#310
cc @runcom @vbatts @wking @stevvooe

wking · 2016-09-16T03:42:00Z

image/manifest_test.go

@@ -122,3 +103,45 @@ func TestUnpackLayer(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func createTarFile(name string, list []tarContent) (descriptor, error) {


We use tar files for image-layout as well. This helper is about creating layers, so it should probably be createLayer or similar to distinguish from image-layout tooling.

And we have a public Descriptor type in github.com/opencontainers/image-spec/specs-go. Can we return that instead of the private, local descriptor (one step closer to being able to drop the local descriptor ;)?

And we have a public Descriptor type in github.com/opencontainers/image-spec/specs-go. Can we return that instead of the private, local descriptor (one step closer to being able to drop the local descriptor ;)?

if it doesn't need to call any of the method attached to descriptor - then yes I would use the public Descriptor as well - otherwise let's keep descriptor for now

wking · 2016-09-16T03:46:19Z

image/manifest_test.go

+		if _, err = io.Copy(tw, bytes.NewReader(content.b)); err != nil {
+			tw.Close()
+			gw.Close()
+			f.Close()


This looks like a job for defer ;).

Perhaps you avoid it because you want to ensure the file is completed and closed before you start hashing it. You could jump through some hoops to get the defered functions to fire, but it's probably easier and more efficient to use something like:

var file *os.File file, err = os.Create(name) // + err checking defer file.Close() hash := sha256.New() counter := NewCountingWriter() // TODO: write this up. It counts bytes written to it and discards the content hashingWriter := io.MultiWriter(file, hash, counter) gzipWriter := gzip.NewWriter(hashingWriter) defer gzipWriter.Close() // create tarWriter and writer content err = tarWriter.Close() // + err checking err = gzipWriter.Close() // + err checking return &specs.Descriptor{ MediaType: v1.MediaTypeImageLayer, Digest: fmt.Sprintf("sha256:%x", h.Sum(nil)), Size: counter.Size, }, nil

On success, the deferred closes might error out (because we've already successfully closed the files). But who cares? We successfully closed the files. On error, we're ignoring possible Close errors. But who cares? We're already returning an error.

wking · 2016-09-16T04:22:55Z

image/image_test.go

+	err = os.MkdirAll(filepath.Join(root, "refs"), 0700)
+	if err != nil {
+		t.Fatal(err)
+	}


This image-layout directory initialization also duplicates existing logic in manifest_test.go. Can we pull it out into createDirectoryImageLayout or some such?

Actually, it looks like manifest_test.go is only stubbing in the blobs directory, but not setting up the rest of image-layout. It would certainly help if we had a createDirectoryImageLayout with the more complete logic you have here so we could have a full image-layout there instead of its current partial format.

wking · 2016-09-16T04:29:10Z

image/image_test.go

+		return descriptor{}, err
+	}
+
+	err = os.Rename(layerPath, filepath.Join(root, "blobs", "sha256", desc.Digest))


manifest_test.go is also doing similar post-creation renaming. Can we use createLayerFile there as well?

wking · 2016-09-16T04:34:15Z

image/image_test.go

+		t.Fatal(err)
+	}
+
+	err = ValidateLayout(root, []string{"latest"}, nil)


It's a bit odd to have latest explicitly here, but to have it implicit in the createRefFile call earlier. Can we have createRefFile(root string, ref string, descriptor *specs.Descriptor) err?

wking · 2016-09-16T09:05:04Z

On Fri, Sep 16, 2016 at 01:01:27AM -0700, Antonio Murdaca wrote:

+func createTarFile(name string, list []tarContent) (descriptor, error) {

And we have a public Descriptor type in
github.com/opencontainers/image-spec/specs-go. Can we return that
instead of the private, local descriptor (one step closer to being
able to drop the local descriptor ;)?

if it doesn't need to call any of the method attached to
descriptor - then yes I would use the public Descriptor as well

otherwise let's keep descriptor for now

I think having public and private descriptor types is more
complication than the convenience of methods is worth. I've converted
the methods to functions in #5 (among other things ;) if you want to
see how it looks.

xiekeyang · 2016-09-18T03:34:56Z

I improve the patch according to reviewer's comments.

set up common functions for creating tar file, image layers and hashed digest blobs.
apply the functions for manifest unit test.
set up unit test for ValidateImageLayout.
set up configurable reference.
make these changes in unit test files, not to join binary compiling. If other implementation need them, it is easy to move the functions to functionality files.

PTAL.

cc @wking @runcom @stevvooe @jonboulle

wking · 2016-09-18T04:04:14Z

image/manifest_test.go

-	// generate sha256 hash
-	h := sha256.New()
-	file, err := os.Open(tarfile)
+	err = os.MkdirAll(filepath.Join(root, "blobs", "sha256"), 0700)


You should use createImageLayoutBundle here to get oci-layout, etc.

It seems some inappropriate to use createImageLayoutBundle here.
the TestUnpackLayer prerequisite is just layer file with right path and its descriptor. It even needn't real manifest file. So it is enough to make layer dir and create layer file.
If TestUnpackLayer function is missing some test case( I read testManifest.unpack() function, and think TestUnpackLayer is fine), it is another story.
if it is correct?

wking · 2016-09-18T04:08:49Z

image/image_test.go

+
+	// generate sha256 hash
+	hash := sha256.New()
+	size, err := io.Copy(hash, file)


This is a second pass through a file that you just streamed to disk. It would be more efficient to hash and size while you were streaming to disk, but as long as this is just for the test suite a second pass is probably fine.

wking · 2016-09-18T04:09:43Z

image/image_test.go

+}
+
+func createRefFile(root string, mft descriptor) error {
+	refpath := filepath.Join(root, "refs", "latest")


"latest" → refTag.

wking · 2016-09-18T07:10:56Z

On Sat, Sep 17, 2016 at 11:16:24PM -0700, xiekeyang wrote:

xiekeyang commented on this pull request.

gw := gzip.NewWriter(f)

- tw := tar.NewWriter(gw)

tw.WriteHeader(&tar.Header{Name: "test", Size: 4, Mode: 0600})

io.Copy(tw, bytes.NewReader([]byte("test")))

tw.Close()

gw.Close()

- f.Close()

// generate sha256 hash

h := sha256.New()

file, err := os.Open(tarfile)

err = os.MkdirAll(filepath.Join(root, "blobs", "sha256"), 0700)

It seems some inappropriate to use createImageLayoutBundle here.
the TestUnpackLayer prerequisite is just layer file with right
path and its descriptor. It even needn't real manifest file. So it
is enough to make layer dir and create layer file.

…/blobs/sha256/… is part of an image-layout, but not a complete
image layout (e.g. it has no oci-layout file). If the unpacker
implementation checked for an oci-layout file to confirm “yes, this is
an image-layout 1.0.0. I can lookup the blob at {algo}/{hash}”, then
it would fail this test as it stands. I think any unpacker which
expects to be using an image-layout should be checking for that
oci-layout file, and consider it a bug that the unpacker apparently
doesn't check. If we use createImageLayoutBundle, then this test will
continue to pass after we fix that bug.

xiekeyang · 2016-09-18T08:09:05Z

@wking

…/blobs/sha256/… is part of an image-layout, but not a complete
image layout (e.g. it has no oci-layout file). If the unpacker
implementation checked for an oci-layout file to confirm “yes, this is
an image-layout 1.0.0. I can lookup the blob at {algo}/{hash}”, then
it would fail this test as it stands. I think any unpacker which
expects to be using an image-layout should be checking for that
oci-layout file, and consider it a bug that the unpacker apparently
doesn't check. If we use createImageLayoutBundle, then this test will
continue to pass after we fix that bug.

I'm all for you. But I honestly dislike to fix other bugs on this patch.
My preference is to commit only image_test.go for image layout validation file on this patch.
And fix manifest unpacking problem and improve the manifest unit test on other patch.
WDYT?

wking · 2016-09-18T08:20:54Z

On Sun, Sep 18, 2016 at 01:09:05AM -0700, xiekeyang wrote:

But I honestly dislike to fix other bugs on this patch. My
preference is to commit only image_test.go for image layout
validation file on this patch. And fix manifest unpacking problem
and improve the manifest unit test on other patch.

It's all going to end up in the same stomach ;). My preference would
be to have multiple commits in this PR, but if you want to punt some
of it to a follow-up PR I'm ok with that. The (small) risk of
splitting this sort of thing is that the original contributor looses
interest after the PR that partially transitions to the new system,
and other folks have to pick up the pieces (and either revert or
complete the transition) on their behalf.

xiekeyang · 2016-09-18T09:28:08Z

@wking
I research func (m *manifest) unpack related functions, find it is just responsible for unpack layers. Before it is called, finding manifest, finding config and manifest validation are always done. So manifest.unpack() needn't to check whole image bundle.
The mechanism may be confused, and need reconstruct, which seems not a bug. I suggest that we had better not to expend this PR scope. And I'd try to improve the mechanism on new PR.
WDYT?

xiekeyang · 2016-09-19T02:46:35Z

cc @runcom @stevvooe @jonboulle @s-urbaniak @wking
PTAL

wking · 2016-09-19T04:39:04Z

On Sun, Sep 18, 2016 at 02:28:09AM -0700, xiekeyang wrote:

Before it is called, finding manifest, finding config and manifest
validation are always done. So manifest.unpack() needn't to check
whole image bundle.

It still needs to pull layers from the image-layout directory, so I
think we want a full image-layout structure. But we're having trouble
convincing each other, so that seems like a good enough reason to
punt. As it stands in 966be8f, I think this PR is generally moving
us in a good direction. If there's too much in the air to be able to
get to the bottom of our differences here, I'm ok picking up those
discussions in follow-up work. And if/when #5 lands I think all of
these image/*_test.go will be using the generic engines, so we won't
have to worry about whether a partially-setup image-layout makes sense
or not.

xiekeyang · 2016-09-21T08:32:55Z

cc @stevvooe @runcom @jonboulle

Signed-off-by: xiekeyang <keyang.xie@gmail.com>

xiekeyang · 2016-09-22T08:19:53Z

As we need to confirm and fix some problem in func (m *manifest) unpack, I'd like to narrow this PR, only for function ValidateLayout, which I think it is OK. And I'm working on the above discussed issue and will present new PR.

cc @wking @stevvooe

wking · 2016-09-22T14:50:28Z

On Thu, Sep 22, 2016 at 01:19:54AM -0700, xiekeyang wrote:

As we need to confirm and fix some problem in func (m *manifest) unpack, I'd like to narrow this PR, only for function
ValidateLayout, which I think it is OK.

f3064f0 → e85687e just dropped all of your changes from
image/manifest_test.go. I think those were solid, useful changes, so
I don't see a need to drop them here. But if the image/image_test.go
stuff lands faster because you've dropped them, I'm ok with the
additional punt.

xiekeyang · 2016-09-23T02:24:44Z

@wking Right. hopefully this image test will stand. And I'll submit manifest test.
cc @s-urbaniak @jonboulle @runcom PTAL.

xiekeyang · 2016-09-26T10:37:05Z

PTAL.

vbatts · 2016-09-27T12:54:39Z

LGTM

Outsource this stuff to avoid duplication of effort. I have no idea why newDescriptor was returning just the hex and createHashedBlob (its only consumer) was fixing that (by the "Normalize the hashed digest" comment). But that is how the image tests have worked since they landed in e85687e (unit test for layout validation, 2016-09-22, opencontainers#4). This commit drops the "Normalization" hack and fixes newDescriptor to actually return a valid digest. Signed-off-by: W. Trevor King <wking@tremily.us>

wking reviewed Sep 16, 2016

View reviewed changes

coolljt0725 mentioned this pull request Sep 18, 2016

Cleanup partially-unpacked directories on failures #7

Merged

wking reviewed Sep 18, 2016

View reviewed changes

xiekeyang mentioned this pull request Sep 19, 2016

independently unpacking layer functions #11

Closed

xiekeyang changed the title ~~migrate #image-spec/pull/310~~ add unit test for ValidateLayout function Sep 19, 2016

unit test for layout validation

e85687e

Signed-off-by: xiekeyang <keyang.xie@gmail.com>

vbatts merged commit ef40309 into opencontainers:master Oct 6, 2016

xiekeyang deleted the unit-test branch October 7, 2016 03:07

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unit test for ValidateLayout function #4

add unit test for ValidateLayout function #4

xiekeyang commented Sep 16, 2016

wking Sep 16, 2016

wking Sep 16, 2016

runcom Sep 16, 2016

wking Sep 16, 2016

wking Sep 16, 2016 •

edited

Loading

wking Sep 16, 2016

wking Sep 16, 2016

wking Sep 16, 2016

wking Sep 16, 2016

wking commented Sep 16, 2016

xiekeyang commented Sep 18, 2016

wking Sep 18, 2016

xiekeyang Sep 18, 2016

wking Sep 18, 2016

wking Sep 18, 2016

xiekeyang Sep 18, 2016

wking commented Sep 18, 2016

- tw := tar.NewWriter(gw)

- f.Close()

xiekeyang commented Sep 18, 2016

wking commented Sep 18, 2016

xiekeyang commented Sep 18, 2016 •

edited

Loading

xiekeyang commented Sep 19, 2016

wking commented Sep 19, 2016

xiekeyang commented Sep 21, 2016

xiekeyang commented Sep 22, 2016

wking commented Sep 22, 2016

xiekeyang commented Sep 23, 2016

xiekeyang commented Sep 26, 2016

vbatts commented Sep 27, 2016

add unit test for ValidateLayout function #4

add unit test for ValidateLayout function #4

Conversation

xiekeyang commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Sep 16, 2016

xiekeyang commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Sep 18, 2016

- tw := tar.NewWriter(gw)

- f.Close()

xiekeyang commented Sep 18, 2016

wking commented Sep 18, 2016

xiekeyang commented Sep 18, 2016 • edited Loading

xiekeyang commented Sep 19, 2016

wking commented Sep 19, 2016

xiekeyang commented Sep 21, 2016

xiekeyang commented Sep 22, 2016

wking commented Sep 22, 2016

xiekeyang commented Sep 23, 2016

xiekeyang commented Sep 26, 2016

vbatts commented Sep 27, 2016

wking Sep 16, 2016 •

edited

Loading

xiekeyang commented Sep 18, 2016 •

edited

Loading