-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use official async-tar #2862
Use official async-tar #2862
Conversation
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Thanks for getting this dep back to upstream!
I thought I did add one but it turns out I was thinking of an e2e test in the containerd-shim-spin repo. So, the quick way to test is to |
PS One other testing note: I believe the app does need at least one static asset with a size over a certain minimum, or else the content is inlined in the oci manifest. update: just tested: size of static asset does not matter, but at least one asset is necessary to test archive layers |
@vdice Okay, I set that environment variable, and did a push from Windows of a test application with one small asset (about a dozen bytes). I was able to spin up from the registry on both Windows and Linux and verify that the application could read the asset. I also downloaded the registry image with a simple OCI client and printed out the layers' media types:
If I remove the environment variable and re-push, the I realise this is a very limited test, but I am going to mark this as ready to review - if there are further tests you'd like me to run then feel free to put the mockers on it. Thank you ever so much for your guidance on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I will note it doesn't fully address the issues laid out in #2846, namely the use of async-tar
in general which brings in async-std
and associated dependencies.
I'd be happy with merging this PR as is but keeping #2846 open so we can discuss whether removing async-tar
entirely (and using tar
along with spawn_blocking
) would be a better way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! All apps I tested with work great.
@rylev I can't find a way to make GitHub unlink the issue but I hear you. I will merge this then reopen your issue. |
Fixes #2846.
This built locally for me on Windows. I'm pushing this to see whether it builds in CI, and if the artefacts are usable.
@vdice The dependency is used only (I believe) in the archive layers stuff (#1847). Do we have a test case I can use to hit that?