From 0b2cc3b09250a3227255c57210df88421a24b603 Mon Sep 17 00:00:00 2001 From: Terry Howe Date: Fri, 26 Jul 2024 13:23:08 -0600 Subject: [PATCH] refactor: Successor getting with separation of concerns Signed-off-by: Terry Howe --- cmd/oras/internal/display/status/text.go | 20 ++- cmd/oras/internal/display/status/text_test.go | 131 ++++++++++++++++++ cmd/oras/internal/display/status/tty.go | 17 ++- cmd/oras/internal/display/status/tty_test.go | 62 --------- cmd/oras/internal/display/status/utils.go | 16 +++ cmd/oras/internal/output/print.go | 30 ---- cmd/oras/root/cp.go | 14 +- internal/descriptor/descriptor.go | 1 - internal/graph/graph.go | 16 +++ internal/graph/graph_test.go | 43 ++++++ 10 files changed, 244 insertions(+), 106 deletions(-) create mode 100644 cmd/oras/internal/display/status/text_test.go diff --git a/cmd/oras/internal/display/status/text.go b/cmd/oras/internal/display/status/text.go index c2f35af62..5e5b6c20d 100644 --- a/cmd/oras/internal/display/status/text.go +++ b/cmd/oras/internal/display/status/text.go @@ -17,13 +17,13 @@ package status import ( "context" + "oras.land/oras/internal/graph" "sync" - "oras.land/oras/cmd/oras/internal/output" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2" "oras.land/oras-go/v2/content" + "oras.land/oras/cmd/oras/internal/output" ) // TextPushHandler handles text status output for push events. @@ -65,9 +65,15 @@ func (ph *TextPushHandler) UpdateCopyOptions(opts *oras.CopyGraphOptions, fetche } opts.PostCopy = func(ctx context.Context, desc ocispec.Descriptor) error { committed.Store(desc.Digest.String(), desc.Annotations[ocispec.AnnotationTitle]) - if err := output.PrintSuccessorStatus(ctx, desc, fetcher, committed, ph.printer.StatusPrinter(PushPromptSkipped)); err != nil { + successors, err := graph.FilteredSuccessors(ctx, desc, fetcher, DeduplicatedFilter(committed)) + if err != nil { return err } + for _, successor := range successors { + if err = ph.printer.PrintStatus(successor, PushPromptSkipped); err != nil { + return err + } + } return ph.printer.PrintStatus(desc, PushPromptUploaded) } } @@ -149,9 +155,15 @@ func (ch *TextCopyHandler) PreCopy(_ context.Context, desc ocispec.Descriptor) e // PostCopy implements PostCopy of CopyHandler. func (ch *TextCopyHandler) PostCopy(ctx context.Context, desc ocispec.Descriptor) error { ch.committed.Store(desc.Digest.String(), desc.Annotations[ocispec.AnnotationTitle]) - if err := output.PrintSuccessorStatus(ctx, desc, ch.fetcher, ch.committed, ch.printer.StatusPrinter(copyPromptSkipped)); err != nil { + successors, err := graph.FilteredSuccessors(ctx, desc, ch.fetcher, DeduplicatedFilter(ch.committed)) + if err != nil { return err } + for _, successor := range successors { + if err = ch.printer.PrintStatus(successor, copyPromptSkipped); err != nil { + return err + } + } return ch.printer.PrintStatus(desc, copyPromptCopied) } diff --git a/cmd/oras/internal/display/status/text_test.go b/cmd/oras/internal/display/status/text_test.go new file mode 100644 index 000000000..47d9d6282 --- /dev/null +++ b/cmd/oras/internal/display/status/text_test.go @@ -0,0 +1,131 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package status + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/content/memory" + "oras.land/oras/cmd/oras/internal/output" + "oras.land/oras/internal/testutils" + "os" + "strings" + "testing" +) + +var ( + ctx context.Context + builder *strings.Builder + printer *output.Printer + bogus ocispec.Descriptor + memStore *memory.Store + memDesc ocispec.Descriptor + manifestDesc ocispec.Descriptor +) + +func TestMain(m *testing.M) { + // memory store for testing + memStore = memory.New() + content := []byte("test") + r := bytes.NewReader(content) + memDesc = ocispec.Descriptor{ + MediaType: "application/octet-stream", + Digest: digest.FromBytes(content), + Size: int64(len(content)), + } + if err := memStore.Push(context.Background(), memDesc, r); err != nil { + fmt.Println("Setup failed:", err) + os.Exit(1) + } + if err := memStore.Tag(context.Background(), memDesc, memDesc.Digest.String()); err != nil { + fmt.Println("Setup failed:", err) + os.Exit(1) + } + + layer1Desc := memDesc + layer1Desc.Annotations = map[string]string{ocispec.AnnotationTitle: "layer1"} + layer2Desc := memDesc + layer2Desc.Annotations = map[string]string{ocispec.AnnotationTitle: "layer2"} + manifest := ocispec.Manifest{ + MediaType: ocispec.MediaTypeImageManifest, + Layers: []ocispec.Descriptor{layer1Desc, layer2Desc}, + Config: memDesc, + } + manifestContent, err := json.Marshal(&manifest) + if err != nil { + fmt.Println("Setup failed:", err) + os.Exit(1) + } + manifestDesc = ocispec.Descriptor{ + MediaType: manifest.MediaType, + Size: int64(len(manifestContent)), + Digest: digest.FromBytes(manifestContent), + } + if err := memStore.Push(context.Background(), manifestDesc, strings.NewReader(string(manifestContent))); err != nil { + fmt.Println("Setup failed:", err) + os.Exit(1) + } + if err := memStore.Tag(context.Background(), memDesc, memDesc.Digest.String()); err != nil { + fmt.Println("Setup failed:", err) + os.Exit(1) + } + + ctx = context.Background() + builder = &strings.Builder{} + printer = output.NewPrinter(builder, os.Stderr, false) + bogus = ocispec.Descriptor{MediaType: ocispec.MediaTypeImageManifest} + m.Run() +} + +func TestTextCopyHandler_OnMounted(t *testing.T) { + fetcher := testutils.NewMockFetcher(t) + ch := NewTextCopyHandler(printer, fetcher.Fetcher) + if ch.OnMounted(ctx, fetcher.OciImage) != nil { + t.Error("OnMounted() should not return an error") + } + +} + +func TestTextCopyHandler_OnCopySkipped(t *testing.T) { + fetcher := testutils.NewMockFetcher(t) + ch := NewTextCopyHandler(printer, fetcher.Fetcher) + if ch.OnCopySkipped(ctx, fetcher.OciImage) != nil { + t.Error("OnCopySkipped() should not return an error") + } +} + +func TestTextCopyHandler_PostCopy(t *testing.T) { + fetcher := testutils.NewMockFetcher(t) + ch := NewTextCopyHandler(printer, fetcher.Fetcher) + if ch.PostCopy(ctx, fetcher.OciImage) != nil { + t.Error("PostCopy() should not return an error") + } + if ch.PostCopy(ctx, bogus) == nil { + t.Error("PostCopy() should return an error") + } +} + +func TestTextCopyHandler_PreCopy(t *testing.T) { + fetcher := testutils.NewMockFetcher(t) + ch := NewTextCopyHandler(printer, fetcher.Fetcher) + if ch.PreCopy(ctx, fetcher.OciImage) != nil { + t.Error("PreCopy() should not return an error") + } +} diff --git a/cmd/oras/internal/display/status/tty.go b/cmd/oras/internal/display/status/tty.go index fa7e4345c..9269717a1 100644 --- a/cmd/oras/internal/display/status/tty.go +++ b/cmd/oras/internal/display/status/tty.go @@ -17,11 +17,10 @@ package status import ( "context" + "oras.land/oras/internal/graph" "os" "sync" - "oras.land/oras/cmd/oras/internal/output" - ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2" "oras.land/oras-go/v2/content" @@ -70,9 +69,17 @@ func (ph *TTYPushHandler) UpdateCopyOptions(opts *oras.CopyGraphOptions, fetcher } opts.PostCopy = func(ctx context.Context, desc ocispec.Descriptor) error { committed.Store(desc.Digest.String(), desc.Annotations[ocispec.AnnotationTitle]) - return output.PrintSuccessorStatus(ctx, desc, fetcher, committed, func(d ocispec.Descriptor) error { - return ph.tracked.Prompt(d, PushPromptSkipped) - }) + successors, err := graph.FilteredSuccessors(ctx, desc, fetcher, DeduplicatedFilter(committed)) + if err != nil { + return err + } + for _, successor := range successors { + err = ph.tracked.Prompt(successor, PushPromptSkipped) + if err != nil { + return err + } + } + return nil } } diff --git a/cmd/oras/internal/display/status/tty_test.go b/cmd/oras/internal/display/status/tty_test.go index cc6fc6935..f611bcd33 100644 --- a/cmd/oras/internal/display/status/tty_test.go +++ b/cmd/oras/internal/display/status/tty_test.go @@ -16,74 +16,12 @@ limitations under the License. package status import ( - "bytes" - "context" - "encoding/json" - "fmt" "os" - "strings" "testing" - "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" - "oras.land/oras-go/v2/content/memory" ) -var ( - memStore *memory.Store - memDesc ocispec.Descriptor - manifestDesc ocispec.Descriptor -) - -func TestMain(m *testing.M) { - // memory store for testing - memStore = memory.New() - content := []byte("test") - r := bytes.NewReader(content) - memDesc = ocispec.Descriptor{ - MediaType: "application/octet-stream", - Digest: digest.FromBytes(content), - Size: int64(len(content)), - } - if err := memStore.Push(context.Background(), memDesc, r); err != nil { - fmt.Println("Setup failed:", err) - os.Exit(1) - } - if err := memStore.Tag(context.Background(), memDesc, memDesc.Digest.String()); err != nil { - fmt.Println("Setup failed:", err) - os.Exit(1) - } - - layer1Desc := memDesc - layer1Desc.Annotations = map[string]string{ocispec.AnnotationTitle: "layer1"} - layer2Desc := memDesc - layer2Desc.Annotations = map[string]string{ocispec.AnnotationTitle: "layer2"} - manifest := ocispec.Manifest{ - MediaType: ocispec.MediaTypeImageManifest, - Layers: []ocispec.Descriptor{layer1Desc, layer2Desc}, - Config: memDesc, - } - manifestContent, err := json.Marshal(&manifest) - if err != nil { - fmt.Println("Setup failed:", err) - os.Exit(1) - } - manifestDesc = ocispec.Descriptor{ - MediaType: manifest.MediaType, - Size: int64(len(manifestContent)), - Digest: digest.FromBytes(manifestContent), - } - if err := memStore.Push(context.Background(), manifestDesc, strings.NewReader(string(manifestContent))); err != nil { - fmt.Println("Setup failed:", err) - os.Exit(1) - } - if err := memStore.Tag(context.Background(), memDesc, memDesc.Digest.String()); err != nil { - fmt.Println("Setup failed:", err) - os.Exit(1) - } - m.Run() -} - func TestTTYPushHandler_OnFileLoading(t *testing.T) { ph := NewTTYPushHandler(os.Stdout) if ph.OnFileLoading("test") != nil { diff --git a/cmd/oras/internal/display/status/utils.go b/cmd/oras/internal/display/status/utils.go index 0b7f244e1..453a1acc6 100644 --- a/cmd/oras/internal/display/status/utils.go +++ b/cmd/oras/internal/display/status/utils.go @@ -15,6 +15,12 @@ limitations under the License. package status +import ( + "sync" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" +) + // Prompts for pull events. const ( PullPromptDownloading = "Downloading" @@ -41,3 +47,13 @@ const ( copyPromptSkipped = "Skipped" copyPromptMounted = "Mounted" ) + +// DeduplicatedFilter filters out deduplicated descriptors. +func DeduplicatedFilter(committed *sync.Map) func(desc ocispec.Descriptor) bool { + return func(desc ocispec.Descriptor) bool { + name := desc.Annotations[ocispec.AnnotationTitle] + v, ok := committed.Load(desc.Digest.String()) + // committed but not printed == deduplicated + return ok && v != name + } +} diff --git a/cmd/oras/internal/output/print.go b/cmd/oras/internal/output/print.go index a3f6a42fc..d7e486866 100644 --- a/cmd/oras/internal/output/print.go +++ b/cmd/oras/internal/output/print.go @@ -16,7 +16,6 @@ limitations under the License. package output import ( - "context" "fmt" "io" "sync" @@ -24,12 +23,8 @@ import ( "oras.land/oras/internal/descriptor" ocispec "github.com/opencontainers/image-spec/specs-go/v1" - "oras.land/oras-go/v2/content" ) -// PrintFunc is the function type returned by StatusPrinter. -type PrintFunc func(ocispec.Descriptor) error - // Printer prints for status handlers. type Printer struct { out io.Writer @@ -92,28 +87,3 @@ func (p *Printer) PrintStatus(desc ocispec.Descriptor, status string) error { } return p.Println(status, descriptor.ShortDigest(desc), name) } - -// StatusPrinter returns a tracking function for transfer status. -func (p *Printer) StatusPrinter(status string) PrintFunc { - return func(desc ocispec.Descriptor) error { - return p.PrintStatus(desc, status) - } -} - -// PrintSuccessorStatus prints transfer status of successors. -func PrintSuccessorStatus(ctx context.Context, desc ocispec.Descriptor, fetcher content.Fetcher, committed *sync.Map, print PrintFunc) error { - successors, err := content.Successors(ctx, fetcher, desc) - if err != nil { - return err - } - for _, s := range successors { - name := s.Annotations[ocispec.AnnotationTitle] - if v, ok := committed.Load(s.Digest.String()); ok && v != name { - // Reprint status for deduplicated content - if err := print(s); err != nil { - return err - } - } - } - return nil -} diff --git a/cmd/oras/root/cp.go b/cmd/oras/root/cp.go index 5f04adf67..9f0fd6fa4 100644 --- a/cmd/oras/root/cp.go +++ b/cmd/oras/root/cp.go @@ -38,7 +38,6 @@ import ( "oras.land/oras/cmd/oras/internal/display/status/track" oerrors "oras.land/oras/cmd/oras/internal/errors" "oras.land/oras/cmd/oras/internal/option" - "oras.land/oras/cmd/oras/internal/output" "oras.land/oras/internal/docker" "oras.land/oras/internal/graph" "oras.land/oras/internal/listener" @@ -198,9 +197,16 @@ func doCopy(ctx context.Context, copyHandler status.CopyHandler, src oras.ReadOn } extendedCopyOptions.PostCopy = func(ctx context.Context, desc ocispec.Descriptor) error { committed.Store(desc.Digest.String(), desc.Annotations[ocispec.AnnotationTitle]) - return output.PrintSuccessorStatus(ctx, desc, tracked, committed, func(desc ocispec.Descriptor) error { - return tracked.Prompt(desc, promptSkipped) - }) + successors, err := graph.FilteredSuccessors(ctx, desc, tracked, status.DeduplicatedFilter(committed)) + if err != nil { + return err + } + for _, successor := range successors { + if err = tracked.Prompt(successor, promptSkipped); err != nil { + return err + } + } + return nil } extendedCopyOptions.OnMounted = func(ctx context.Context, desc ocispec.Descriptor) error { committed.Store(desc.Digest.String(), desc.Annotations[ocispec.AnnotationTitle]) diff --git a/internal/descriptor/descriptor.go b/internal/descriptor/descriptor.go index 46ad93249..84806fed2 100644 --- a/internal/descriptor/descriptor.go +++ b/internal/descriptor/descriptor.go @@ -18,7 +18,6 @@ package descriptor import ( "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" - "oras.land/oras/internal/docker" ) diff --git a/internal/graph/graph.go b/internal/graph/graph.go index acaf42171..5041c2a2d 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -130,3 +130,19 @@ func FindPredecessors(ctx context.Context, src oras.ReadOnlyGraphTarget, descs [ } return referrers, nil } + +// FilteredSuccessors fetches successors and returns filtered ones. +func FilteredSuccessors(ctx context.Context, desc ocispec.Descriptor, fetcher content.Fetcher, filter func(ocispec.Descriptor) bool) ([]ocispec.Descriptor, error) { + allSuccessors, err := content.Successors(ctx, fetcher, desc) + if err != nil { + return nil, err + } + + var successors []ocispec.Descriptor + for _, s := range allSuccessors { + if filter(s) { + successors = append(successors, s) + } + } + return successors, nil +} diff --git a/internal/graph/graph_test.go b/internal/graph/graph_test.go index 15efd94f9..9cd95b2ee 100644 --- a/internal/graph/graph_test.go +++ b/internal/graph/graph_test.go @@ -68,3 +68,46 @@ func TestSuccessors(t *testing.T) { }) } } + +func TestDescriptor_GetSuccessors(t *testing.T) { + mockFetcher := testutils.NewMockFetcher(t) + + allFilter := func(ocispec.Descriptor) bool { + return true + } + got, err := FilteredSuccessors(context.Background(), mockFetcher.OciImage, mockFetcher.Fetcher, allFilter) + if nil != err { + t.Errorf("FilteredSuccessors unexpected error %v", err) + } + if len(got) != 2 { + t.Errorf("Expected 2 successors got %v", len(got)) + } + if mockFetcher.Subject.Digest != got[0].Digest { + t.Errorf("FilteredSuccessors got %v, want %v", got[0], mockFetcher.Subject) + } + if mockFetcher.Config.Digest != got[1].Digest { + t.Errorf("FilteredSuccessors got %v, want %v", got[1], mockFetcher.Subject) + } + + noConfig := func(desc ocispec.Descriptor) bool { + return desc.Digest != mockFetcher.Config.Digest + } + got, err = FilteredSuccessors(context.Background(), mockFetcher.OciImage, mockFetcher.Fetcher, noConfig) + if nil != err { + t.Errorf("FilteredSuccessors unexpected error %v", err) + } + if len(got) != 1 { + t.Errorf("Expected 1 successors got %v", len(got)) + } + if mockFetcher.Subject.Digest != got[0].Digest { + t.Errorf("FilteredSuccessors got %v, want %v", got[0], mockFetcher.Subject) + } + + got, err = FilteredSuccessors(context.Background(), ocispec.Descriptor{MediaType: ocispec.MediaTypeImageManifest}, mockFetcher.Fetcher, allFilter) + if nil == err { + t.Error("FilteredSuccessors expected error") + } + if got != nil { + t.Errorf("FilteredSuccessors unexpected %v", got) + } +}