-
Notifications
You must be signed in to change notification settings - Fork 39
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
Implement gz compression #153
Conversation
Add test for UncompressedDigest Fix dotnet#29
Fix tests
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.
Thanks!
Are there instructions to manually test this out myself? Or are the automated tests good enough? |
@benvillalobos do you have the manual use-just-built-package steps written down? If not can you? |
However I'd expect the push-to-local-Docker-registry tests to exercise this codepath--if you can validate that with a breakpoint @eerhardt I think we'd be good to move forward. |
We should add something to to devguide here, but my overall process is:
one downside to this is that you pollute your global nuget cache with the intermediate packages, but that's a failing of the overall nuget cache design and not something we can really mitigate. just gotta purge it every so often I suppose. |
This test was definitely broken when my code wasn't working: sdk-container-builds/Test.Microsoft.NET.Build.Containers.Filesystem/EndToEnd.cs Lines 15 to 50 in da83f2f
I can try out these steps and submit a PR to update the dev guide. |
@rainersigwald I do not! The steps would be in our E2E tests. Something very close to:
This is how I went about testing before we had UT's, starting from scratch. cc @eerhardt edit: looks like I missed #154 :) |
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. Nothing blocking. Left 3 questions.
// Docker treats a COPY instruction that copies to a path like `/app` by | ||
// including `app/` as a directory, with no leading slash. Emulate that here. |
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.
@rainersigwald I know this was before the PR, but I'm not understanding this comment clearly: Is /app
being prefixed or suffixed to the file path?
I ask because if it's a prefix, and it needs to be removed, we could try using the stream-based TarFile.CreateFromDirectory
with includeBaseDirectory=false
, see if that works.
If that's not what the comment meant, then nothing to do here.
I was able to test this end-to-end with a basic ASP.NET app that uses ML.NET (using the above instructions). Using the Using my local build of this PR with the exact same app, the |
Any thoughts on when this can be merged? |
This LGTM and @rainersigwald approved, so I'm merging. |
Compressing the
.tar
files to make the layer files smaller.Fix #29
cc @rainersigwald
Are there other tests I should be adding / modifying?