From 039c06184238b3e3c71f7f0ca7199b3623de0f87 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Wed, 15 Jul 2020 09:21:27 -0600 Subject: [PATCH 1/3] mutate: Add() should tell you what it added Right now, there's no way to find out the resulting descriptor that was added to a mutator unless you Commit() it and look up the result. Let's return the descriptor that was added to allow people to reason about it without having to write even more to disk. Signed-off-by: Tycho Andersen --- cmd/umoci/insert.go | 2 +- cmd/umoci/raw-add-layer.go | 2 +- mutate/mutate.go | 14 ++++++++------ mutate/mutate_test.go | 9 +++++++-- repack.go | 2 +- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/cmd/umoci/insert.go b/cmd/umoci/insert.go index 1785e7edf..ddf7cc502 100644 --- a/cmd/umoci/insert.go +++ b/cmd/umoci/insert.go @@ -189,7 +189,7 @@ func insert(ctx *cli.Context) error { // TODO: We should add a flag to allow for a new layer to be made // non-distributable. - if err := mutator.Add(context.Background(), reader, history); err != nil { + if _, err := mutator.Add(context.Background(), reader, history); err != nil { return errors.Wrap(err, "add diff layer") } diff --git a/cmd/umoci/raw-add-layer.go b/cmd/umoci/raw-add-layer.go index 9d4a46821..0ff8a2bab 100644 --- a/cmd/umoci/raw-add-layer.go +++ b/cmd/umoci/raw-add-layer.go @@ -155,7 +155,7 @@ func rawAddLayer(ctx *cli.Context) error { // TODO: We should add a flag to allow for a new layer to be made // non-distributable. - if err := mutator.Add(context.Background(), newLayer, history); err != nil { + if _, err := mutator.Add(context.Background(), newLayer, history); err != nil { return errors.Wrap(err, "add diff layer") } diff --git a/mutate/mutate.go b/mutate/mutate.go index 6330efd12..11f9b98e6 100644 --- a/mutate/mutate.go +++ b/mutate/mutate.go @@ -280,24 +280,26 @@ func (m *Mutator) add(ctx context.Context, reader io.Reader, history *ispec.Hist // generate the DiffIDs for the image metatadata. The provided history entry is // appended to the image's history and should correspond to what operations // were made to the configuration. -func (m *Mutator) Add(ctx context.Context, r io.Reader, history *ispec.History) error { +func (m *Mutator) Add(ctx context.Context, r io.Reader, history *ispec.History) (ispec.Descriptor, error) { + desc := ispec.Descriptor{} if err := m.cache(ctx); err != nil { - return errors.Wrap(err, "getting cache failed") + return desc, errors.Wrap(err, "getting cache failed") } digest, size, err := m.add(ctx, r, history) if err != nil { - return errors.Wrap(err, "add layer") + return desc, errors.Wrap(err, "add layer") } // Append to layers. - m.manifest.Layers = append(m.manifest.Layers, ispec.Descriptor{ + desc = ispec.Descriptor{ // TODO: Detect whether the layer is gzip'd or not... MediaType: ispec.MediaTypeImageLayerGzip, Digest: digest, Size: size, - }) - return nil + } + m.manifest.Layers = append(m.manifest.Layers, desc) + return desc, nil } // AddNonDistributable is the same as Add, except it adds a non-distributable diff --git a/mutate/mutate_test.go b/mutate/mutate_test.go index 6a60135e6..3ef10753b 100644 --- a/mutate/mutate_test.go +++ b/mutate/mutate_test.go @@ -215,9 +215,10 @@ func TestMutateAdd(t *testing.T) { buffer := bytes.NewBufferString("contents") // Add a new layer. - if err := mutator.Add(context.Background(), buffer, &ispec.History{ + newLayerDesc, err := mutator.Add(context.Background(), buffer, &ispec.History{ Comment: "new layer", - }); err != nil { + }) + if err != nil { t.Fatalf("unexpected error adding layer: %+v", err) } @@ -251,6 +252,10 @@ func TestMutateAdd(t *testing.T) { t.Errorf("manifest.Layers[1].Digest is not the same!") } + if mutator.manifest.Layers[1].Digest != newLayerDesc.Digest { + t.Fatalf("unexpected digest for new layer: %v %v", mutator.manifest.Layers[1].Digest, newLayerDesc.Digest) + } + // Check layer was added. if len(mutator.manifest.Layers) != 2 { t.Errorf("manifest.Layers was not updated") diff --git a/repack.go b/repack.go index c00953bf1..457e36c29 100644 --- a/repack.go +++ b/repack.go @@ -110,7 +110,7 @@ func Repack(engineExt casext.Engine, tagName string, bundlePath string, meta Met // TODO: We should add a flag to allow for a new layer to be made // non-distributable. - if err := mutator.Add(context.Background(), reader, history); err != nil { + if _, err := mutator.Add(context.Background(), reader, history); err != nil { return errors.Wrap(err, "add diff layer") } } From 89fda3a0c27398fae26c99b004150efb813c94f6 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Wed, 15 Jul 2020 14:42:48 -0600 Subject: [PATCH 2/3] mutator: add a way to access the current manifest Note that this is a copy, so you can't actually make a change to the original without using the real Add() etc. methods. Signed-off-by: Tycho Andersen --- mutate/mutate.go | 11 +++++++++++ mutate/mutate_test.go | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/mutate/mutate.go b/mutate/mutate.go index 11f9b98e6..41612ebcd 100644 --- a/mutate/mutate.go +++ b/mutate/mutate.go @@ -155,6 +155,17 @@ func (m *Mutator) Config(ctx context.Context) (ispec.ImageConfig, error) { return m.config.Config, nil } +// Manifest returns the current (cached) image manifest. This is what will be +// appended to when any additional Add() calls are made, and what will be +// Commit()ed if no further changes are made. +func (m *Mutator) Manifest(ctx context.Context) (ispec.Manifest, error) { + if err := m.cache(ctx); err != nil { + return ispec.Manifest{}, errors.Wrap(err, "getting cache failed") + } + + return *m.manifest, nil +} + // Meta returns the current (cached) image metadata, which should be used as // the source for any modifications of the configuration using Set. func (m *Mutator) Meta(ctx context.Context) (Meta, error) { diff --git a/mutate/mutate_test.go b/mutate/mutate_test.go index 3ef10753b..f55cd5cf6 100644 --- a/mutate/mutate_test.go +++ b/mutate/mutate_test.go @@ -256,6 +256,15 @@ func TestMutateAdd(t *testing.T) { t.Fatalf("unexpected digest for new layer: %v %v", mutator.manifest.Layers[1].Digest, newLayerDesc.Digest) } + manifestFromFunction, err := mutator.Manifest(context.Background()) + if err != nil { + t.Fatalf("unexpected error getting manifest: %+v", err) + } + + if !reflect.DeepEqual(manifestFromFunction, *mutator.manifest) { + t.Fatalf("mutator.Manifest() didn't return the cached manifest") + } + // Check layer was added. if len(mutator.manifest.Layers) != 2 { t.Errorf("manifest.Layers was not updated") From d12a48a6ab95e000ae3c31741b8271adf8194571 Mon Sep 17 00:00:00 2001 From: Tycho Andersen Date: Tue, 28 Jul 2020 08:50:55 -0600 Subject: [PATCH 3/3] mutator: AddNonDistributable should tell you what it added too Similar to the previous patch where Add() tells you what it added. Signed-off-by: Tycho Andersen --- mutate/mutate.go | 14 ++++++++------ mutate/mutate_test.go | 8 ++++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/mutate/mutate.go b/mutate/mutate.go index 41612ebcd..ca9a9f1fe 100644 --- a/mutate/mutate.go +++ b/mutate/mutate.go @@ -315,24 +315,26 @@ func (m *Mutator) Add(ctx context.Context, r io.Reader, history *ispec.History) // AddNonDistributable is the same as Add, except it adds a non-distributable // layer to the image. -func (m *Mutator) AddNonDistributable(ctx context.Context, r io.Reader, history *ispec.History) error { +func (m *Mutator) AddNonDistributable(ctx context.Context, r io.Reader, history *ispec.History) (ispec.Descriptor, error) { + desc := ispec.Descriptor{} if err := m.cache(ctx); err != nil { - return errors.Wrap(err, "getting cache failed") + return desc, errors.Wrap(err, "getting cache failed") } digest, size, err := m.add(ctx, r, history) if err != nil { - return errors.Wrap(err, "add non-distributable layer") + return desc, errors.Wrap(err, "add non-distributable layer") } // Append to layers. - m.manifest.Layers = append(m.manifest.Layers, ispec.Descriptor{ + desc = ispec.Descriptor{ // TODO: Detect whether the layer is gzip'd or not... MediaType: ispec.MediaTypeImageLayerNonDistributableGzip, Digest: digest, Size: size, - }) - return nil + } + m.manifest.Layers = append(m.manifest.Layers, desc) + return desc, nil } // Commit writes all of the temporary changes made to the configuration, diff --git a/mutate/mutate_test.go b/mutate/mutate_test.go index f55cd5cf6..dc38b7143 100644 --- a/mutate/mutate_test.go +++ b/mutate/mutate_test.go @@ -309,9 +309,10 @@ func TestMutateAddNonDistributable(t *testing.T) { buffer := bytes.NewBufferString("contents") // Add a new layer. - if err := mutator.AddNonDistributable(context.Background(), buffer, &ispec.History{ + newLayerDesc, err := mutator.AddNonDistributable(context.Background(), buffer, &ispec.History{ Comment: "new layer", - }); err != nil { + }) + if err != nil { t.Fatalf("unexpected error adding layer: %+v", err) } @@ -344,6 +345,9 @@ func TestMutateAddNonDistributable(t *testing.T) { if mutator.manifest.Layers[1].Digest == expectedLayerDigest { t.Errorf("manifest.Layers[1].Digest is not the same!") } + if mutator.manifest.Layers[1].Digest != newLayerDesc.Digest { + t.Fatalf("unexpected digest for new layer: %v %v", mutator.manifest.Layers[1].Digest, newLayerDesc.Digest) + } // Check layer was added. if len(mutator.manifest.Layers) != 2 {