feat(copy): introduces CopyGraphOptions with events support #145

leonardochaia · 2024-09-26T01:59:18Z

What this PR does / why we need it

This PR improves CopyAsync and CopyGraphAsync to include the CopyGraphOptions, inspired from oras-go.

The current implementation:

adds support for CopyOptions events, like OnPreCopy, these were implemented in the form of Action<T>s

Pending items

Define Copy signature

Which issue(s) this PR resolves / fixes

Please check the following list

Does the affected code have corresponding tests, e.g. unit test, E2E test?
Does this change require a documentation update?
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have an appropriate license header?

…ries Signed-off-by: Leonardo Chaia <[email protected]>

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia · 2024-09-26T02:04:17Z

This is a work in progress. Seems to be working running locally. Tests are failing and need more work.

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia · 2024-10-18T10:49:21Z

Hi. I propose concurrency support is implemented in a separate PR. Otherwise this should be ready to review :+

Leo.

leonardochaia · 2024-10-18T11:26:31Z

Example usage:

List<string> knownReferences = [];
foreach (var toReplicate in toReplicateArray)
{
    var copyOpts = new CopyOptions()
    {
        MountFrom = _ => knownReferences.ToArray(),
    };

    copyOpts.OnCopySkipped += d => this.logger.LogTrace("{Digest}: Already exists.", d.Digest);
    copyOpts.OnPreCopy += d => this.logger.LogTrace("{Digest}: Copying..", d.Digest);
    copyOpts.OnPostCopy += d => this.logger.LogTrace("{Digest}: Copied", d.Digest);
    copyOpts.OnMounted += (d, from) => this.logger.LogTrace("{Digest}: Mounted from {From}", d.Digest, from);

    await sourceRepository.CopyAsync(
        toReplicate.SourceImage,
        destinationRepository,
        toReplicate.DestinationImage,
        cancellationToken,
        copyOpts);

    knownReferences.Add(toReplicate.DestinationImage);
}

leonardochaia · 2024-10-25T11:38:01Z

Hi all, any feedback is appreciated 🥇

Thanks!
Leo.

codecov · 2024-10-28T03:35:01Z

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.25%. Comparing base (97f1225) to head (20ac068).

Files with missing lines	Patch %	Lines
src/OrasProject.Oras/AsyncInvocationExtensions.cs	62.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
+ Coverage   89.06%   89.25%   +0.19%     
==========================================
  Files          39       41       +2     
  Lines        1591     1620      +29     
  Branches      204      211       +7     
==========================================
+ Hits         1417     1446      +29     
+ Misses        113      112       -1     
- Partials       61       62       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Wwwsylvia · 2024-10-28T03:46:37Z

Hi @leonardochaia , thank you for your contribution! Sorry for the delay. We need a bit more time to review your PR, but we’ll provide feedback as soon as we can. Thank you for your understanding!

src/OrasProject.Oras/Extensions.cs

src/OrasProject.Oras/Registry/IMounter.cs

src/OrasProject.Oras/Registry/Remote/Repository.cs

src/OrasProject.Oras/Registry/IMounter.cs

src/OrasProject.Oras/Extensions.cs

Wwwsylvia · 2024-11-11T09:04:14Z

Hi. I propose concurrency support is implemented in a separate PR. Otherwise this should be ready to review :+

Do we want to move the mounting support to a separate PR as well?

leonardochaia · 2024-11-11T10:08:16Z

Hi. I propose concurrency support is implemented in a separate PR. Otherwise this should be ready to review :+

Do we want to move the mounting support to a separate PR as well?

Hi @Wwwsylvia , thanks for the review. I'll apply the changes and report back.
We can move the mounting to a separate PR. In such case we would leave only the CopyOptions and related events on this PR, and implement the IMounter and mounting support on copy on a separate PR.

My goal is getting the mounting support when copying graphs since we are using this to replicate a bunch of images between registries and need to make it faster.

Wwwsylvia · 2024-11-11T10:13:28Z

@leonardochaia Sounds good. Thank you!

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia · 2024-11-11T10:42:22Z

PR simplified to only introduce the CopyGraphOptions. Next step is to update the tests to cover the events.

Separate PRs/issues to be created:

Copy to support concurrency
Repository to support mounting
Copy to support mounting

leonardochaia · 2024-11-11T10:53:47Z

Usually CancellationToken is left as the last parameter, however, that would introduce a breaking change. Are we good with introducing a breaking change?

public static async Task<Descriptor> CopyAsync(this ITarget src, string srcRef, ITarget dst, string dstRef, CopyGraphOptions? copyOptions = default, CancellationToken cancellationToken = default)
    
public static async Task CopyGraphAsync(this ITarget src, ITarget dst, Descriptor node, , CopyGraphOptions? copyOptions = default, CancellationToken cancellationToken)

EDIT: Disregard, I've added an overload to keep the CancellationToken last but also support the current signature

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia · 2024-11-11T12:13:56Z

Quick update, I've converted this PR to only include the CopyGraphOptions with events support and corresponding tests.

Next steps:

Add support for mounting to Repository feat(repository): mounting support #152
Add support for mounting to Copy (after this and feat(repository): mounting support #152 are merged)

src/OrasProject.Oras/Extensions.cs

src/OrasProject.Oras/CopyGraphOptions.cs

…us event handlers. Signed-off-by: Leonardo Chaia <[email protected]>

Signed-off-by: Leonardo Chaia <[email protected]>

src/OrasProject.Oras/CopyGraphOptions.cs

Wwwsylvia · 2025-01-13T04:01:22Z

Hi @leonardochaia , happy new year! Would you like to continue our discussion on this?

Wwwsylvia · 2025-04-30T09:10:00Z

We can merge this first and iterate it later.

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia · 2025-05-14T11:11:23Z

I will make changes based on this branch.

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Copilot

Pull Request Overview

This PR introduces enhancements to the copy functionality by adding CopyGraphOptions with event support for both synchronous and asynchronous operations. Key changes include:

Extending CopyAsync and CopyGraphAsync to support new CopyOptions and CopyGraphOptions (with events hooks).
Adding new structs (CopyOptions and CopyGraphOptions) and a helper extension (AsyncInvocationExtensions) to safely invoke asynchronous event delegates.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/OrasProject.Oras.Tests/CopyTest.cs	Added multiple tests to validate copy operations with event hooks and platform filtering.
src/OrasProject.Oras/Extensions.cs	Modified CopyAsync and CopyGraphAsync signatures to accept CopyOptions and CopyGraphOptions.
src/OrasProject.Oras/CopyOptions.cs	Introduced CopyOptions struct to encapsulate CopyGraphOptions.
src/OrasProject.Oras/CopyGraphOptions.cs	Added CopyGraphOptions struct containing pre/post copy and skipped events.
src/OrasProject.Oras/AsyncInvocationExtensions.cs	Provided an extension for invoking asynchronous event delegates using new C# syntax.

Comments suppressed due to low confidence (1)

tests/OrasProject.Oras.Tests/CopyTest.cs:153

[nitpick] Consider renaming the test 'TestCopyExistedRoot' to 'TestCopyExistingRoot' for improved clarity.

[Fact] public async Task TestCopyExistedRoot()

tests/OrasProject.Oras.Tests/CopyTest.cs

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Copilot

Pull Request Overview

This PR introduces event-driven graph copy options, enhancing the CopyAsync and CopyGraphAsync APIs with pre/post copy and skip hooks, and adds supporting types and tests.

Added CopyOptions and CopyGraphOptions structs with event delegates.
Updated Extensions.CopyAsync and CopyGraphAsync to invoke the new event hooks.
Introduced AsyncInvocationExtensions for safe parallel invocation of async event handlers.
Expanded unit tests in CopyTest.cs to cover skip, pre-copy, and post-copy events, and tracking behavior.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/OrasProject.Oras.Tests/CopyTest.cs	Added tests for skip, pre-copy, and post-copy event handlers; implemented `StorageTracker`.
src/OrasProject.Oras/Extensions.cs	Overloaded `CopyAsync`/`CopyGraphAsync` to accept options and trigger `CopyGraphOptions` events.
src/OrasProject.Oras/CopyOptions.cs	Introduced `CopyOptions` struct wrapping `CopyGraphOptions`.
src/OrasProject.Oras/CopyGraphOptions.cs	Defined `CopyGraphOptions` with sync/async event delegates and internal invocation methods.
src/OrasProject.Oras/AsyncInvocationExtensions.cs	Added `InvokeAsync` extension to safely call async event delegates in parallel.

tests/OrasProject.Oras.Tests/CopyTest.cs

src/OrasProject.Oras/AsyncInvocationExtensions.cs

src/OrasProject.Oras/Extensions.cs

shizhMSFT

LGTM

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia · 2025-05-15T04:10:34Z

Refactored the code a bit more, please take a look again @leonardochaia @shizhMSFT @pat-pan

Copilot

Pull Request Overview

This PR introduces CopyGraphOptions with event support to improve the copy operations. Key changes include:

New CopyGraphOptions and CopyOptions classes supporting pre-/post-copy and copy-skipped event hooks.
Updated Extensions methods to pass and use these options.
Expanded tests to verify the behavior of copy operations with the new event hooks.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/OrasProject.Oras.Tests/CopyTest.cs	Added tests for full graph copy, copying an existing root, and copy operations using hooks.
src/OrasProject.Oras/Extensions.cs	Updated copy method signatures to accept new CopyOptions and CopyGraphOptions, and integrated event hook calls.
src/OrasProject.Oras/CopyOptions.cs	Introduced CopyOptions as a subclass of CopyGraphOptions.
src/OrasProject.Oras/CopyGraphOptions.cs	Added a new class that facilitates pre-/post-copy and copy-skipped events with asynchronous support.
src/OrasProject.Oras/AsyncInvocationExtensions.cs	Added extension methods to safely invoke event delegates asynchronously.

Copilot · 2025-05-15T04:11:04Z

tests/OrasProject.Oras.Tests/CopyTest.cs

+        public int PushCount { get; private set; }
+        public int ExistsCount { get; private set; }
+
+        public IList<string> Fetched { get; } = [];


The property 'Fetched' is initialized with an empty array literal, which results in an immutable collection. Consider initializing it with a modifiable collection such as 'new List()' to allow dynamic additions.

Suggested change

public IList<string> Fetched { get; } = [];

public IList<string> Fetched { get; } = new List<string>();

shizhMSFT

LGTM

pat-pan · 2025-05-15T04:51:55Z

src/OrasProject.Oras/AsyncInvocationExtensions.cs

+        return eventDelegate
+            .GetInvocationList()
+            .Select(d => d is Func<TEventArgs, Task> handler ? handler(args) : Task.CompletedTask)
+            .ToArray();


Could the preCopy/postCopy/CopySkip turn into a multicast list?

pat-pan · 2025-05-15T04:52:52Z

src/OrasProject.Oras/CopyGraphOptions.cs

+    /// <summary>
+    /// PreCopy handles the current descriptor before it is copied.
+    /// </summary>
+    public event Action<Descriptor>? PreCopy;


just curious about the rationale behind providing this synchronous version of handlers

It's more convenient to use and users don't have to return Tasks.CompletedTask if they do something synchonously.
See #145 (comment)

pat-pan · 2025-05-15T04:53:07Z

src/OrasProject.Oras/CopyOptions.cs

+/// <summary>
+/// CopyOptions contains parameters for <see cref="Extensions.CopyAsync(OrasProject.Oras.ITarget,string,OrasProject.Oras.ITarget,string,System.Threading.CancellationToken)"/>
+/// </summary>
+public class CopyOptions : CopyGraphOptions { }


Would composition make more sense here? Otherwise, it seems like CopyOptions doesn't do much here. Just wonder why it is needed here

For now, CopyOptions is just a placeholder. We will add more options into it.
I think inheritance makes it easier for users to initialize and use. They can just do:

var opts = new CopyOptions(); await CopyAsync(src, srcRef, dst, dstRef, opts); await CopyGraphAsync(src, dst, root, opts);

and don't need to do:

var opts = new CopyOptions() { CopyGraphOptions = new CopyGraphOptions() }; await CopyAsync(src, srcRef, dst, dstRef, opts); await CopyGraphAsync(src, dst, root, opts.CopyGraphOptions);

Wwwsylvia · 2025-05-20T10:23:36Z

src/OrasProject.Oras/CopyGraphOptions.cs

+    public event Func<Descriptor, Task>? PreCopyAsync;
+
+    /// <summary>
+    /// PreCopy handles the current descriptor before it is copied.
+    /// </summary>
+    public event Action<Descriptor>? PreCopy;
+
+    /// <summary>
+    /// PostCopyAsync handles the current descriptor after it is copied.
+    /// </summary>
+    public event Func<Descriptor, Task>? PostCopyAsync;
+
+    /// <summary>
+    /// PostCopy handles the current descriptor after it is copied.
+    /// </summary>
+    public event Action<Descriptor>? PostCopy;
+
+    /// <summary>
+    /// CopySkippedAsync will be called when the sub-DAG rooted by the current node
+    /// is skipped.
+    /// </summary>
+    public event Func<Descriptor, Task>? CopySkippedAsync;
+
+    /// <summary>
+    /// CopySkipped will be called when the sub-DAG rooted by the current node
+    /// is skipped.
+    /// </summary>
+    public event Action<Descriptor>? CopySkipped;


Just realized that the event handlers cannot be overwritten, which might not meet our need.

leonardochaia changed the title ~~feat(copy): support mounting existing descriptors from other repos~~ feat(copy): makes copy more efficient Sep 26, 2024

feat(copy): support mounting existing descriptors from other reposito…

6817504

…ries Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia force-pushed the feat/improve-copy-performance branch from 847ecb0 to 6817504 Compare September 26, 2024 02:00

fix: standard header

b453fbc

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia and others added 3 commits October 2, 2024 22:14

chore: adds repository tests

6a7a1e9

Signed-off-by: Leonardo Chaia <[email protected]>

fix: copy test

88c6793

Signed-off-by: Leonardo Chaia <[email protected]>

Merge branch 'main' into feat/improve-copy-performance

4c52467

leonardochaia marked this pull request as ready for review October 17, 2024 18:41

leonardochaia requested review from sammychinedu2ky, shizhMSFT and Wwwsylvia as code owners October 17, 2024 18:41

Wwwsylvia reviewed Nov 11, 2024

View reviewed changes

leonardochaia and others added 3 commits November 11, 2024 07:16

Merge branch 'main' into feat/improve-copy-performance

36342c9

chore: removes mounting support to be implemented in separate PR

dbe3509

Signed-off-by: Leonardo Chaia <[email protected]>

refactor: renames class and events as per review

571820c

Signed-off-by: Leonardo Chaia <[email protected]>

chore: adds copy tests

138120e

Signed-off-by: Leonardo Chaia <[email protected]>

leonardochaia changed the title ~~feat(copy): makes copy more efficient~~ feat(copy): introduces CopyGraphOptions with events support Nov 11, 2024

chore: adds overload to prevent breaking change

77e8079

Signed-off-by: Leonardo Chaia <[email protected]>

shizhMSFT reviewed Nov 25, 2024

View reviewed changes

src/OrasProject.Oras/Extensions.cs Outdated Show resolved Hide resolved

src/OrasProject.Oras/CopyGraphOptions.cs Show resolved Hide resolved

leonardochaia added 3 commits November 26, 2024 18:35

feat: introduces InvokeAsync extension method to support asynchrono…

68220bd

…us event handlers. Signed-off-by: Leonardo Chaia <[email protected]>

refactor: delegate InvokeAsync to execute handlers in parallel

e35fc7e

Signed-off-by: Leonardo Chaia <[email protected]>

refactor: Copy events to include sync and async variants.

827f62b

Signed-off-by: Leonardo Chaia <[email protected]>

Wwwsylvia reviewed Dec 18, 2024

View reviewed changes

src/OrasProject.Oras/CopyGraphOptions.cs Outdated Show resolved Hide resolved

src/OrasProject.Oras/CopyGraphOptions.cs Outdated Show resolved Hide resolved

Wwwsylvia added 2 commits April 30, 2025 17:10

Merge branch 'main' into feat/improve-copy-performance

6d1b0ba

address comments

2372ac6

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia added 2 commits May 14, 2025 21:45

fix tests

f7b326d

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

rename

fe10a1a

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia requested a review from Copilot May 14, 2025 13:56

Copilot AI reviewed May 14, 2025

View reviewed changes

tests/OrasProject.Oras.Tests/CopyTest.cs Outdated Show resolved Hide resolved

nit

c1c7182

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia requested review from Wwwsylvia and Copilot May 14, 2025 14:04

Merge branch 'main' into feat/improve-copy-performance

7b31ec3

Copilot AI reviewed May 14, 2025

View reviewed changes

tests/OrasProject.Oras.Tests/CopyTest.cs Show resolved Hide resolved

src/OrasProject.Oras/AsyncInvocationExtensions.cs Show resolved Hide resolved

src/OrasProject.Oras/Extensions.cs Show resolved Hide resolved

shizhMSFT approved these changes May 15, 2025

View reviewed changes

Wwwsylvia added 2 commits May 15, 2025 12:00

refactor

b9f93d0

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

update tests

20ac068

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>

Wwwsylvia requested review from Copilot, shizhMSFT and pat-pan May 15, 2025 04:10

Copilot AI reviewed May 15, 2025

View reviewed changes

shizhMSFT approved these changes May 15, 2025

View reviewed changes

pat-pan suggested changes May 15, 2025

View reviewed changes

Wwwsylvia requested changes May 20, 2025

View reviewed changes

	public IList<string> Fetched { get; } = [];
	public IList<string> Fetched { get; } = new List<string>();

feat(copy): introduces CopyGraphOptions with events support #145

Are you sure you want to change the base?

feat(copy): introduces CopyGraphOptions with events support #145

Uh oh!

Conversation

leonardochaia commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

Pending items

Which issue(s) this PR resolves / fixes

Please check the following list

Uh oh!

leonardochaia commented Sep 26, 2024

Uh oh!

leonardochaia commented Oct 18, 2024

Uh oh!

leonardochaia commented Oct 18, 2024

Uh oh!

leonardochaia commented Oct 25, 2024

Uh oh!

codecov bot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Wwwsylvia commented Oct 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wwwsylvia commented Nov 11, 2024

Uh oh!

leonardochaia commented Nov 11, 2024

Uh oh!

Wwwsylvia commented Nov 11, 2024

Uh oh!

leonardochaia commented Nov 11, 2024

Uh oh!

leonardochaia commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardochaia commented Nov 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wwwsylvia commented Jan 13, 2025

Uh oh!

Wwwsylvia commented Apr 30, 2025

Uh oh!

Wwwsylvia commented May 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia commented May 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 15, 2025

Choose a reason for hiding this comment

leonardochaia commented Sep 26, 2024 •

edited

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

leonardochaia commented Nov 11, 2024 •

edited

Loading