-
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
Extend Layer Packaging for Windows Containers #343
Conversation
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.
The change looks good to me, though tests should be changed as we moved to xunit.
Please let me know if you need help with it.
It would be good to have an e2e test on Windows as you mentioned but it's not possible at the moment. Have you verified the change manually?
I will take care of the test updates, I saw the separate PR with XUnit but at the time of opening this one it wasn't merged yet 😁
Good point. I had checked my changes on a different branch and reported my findings here: #117 (comment) but to be sure nothing changed since my initial findings, I will verify the changes again using this branch. |
{ | ||
RuntimeIdentifier = runtimeIdentifier; |
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.
We should not need this - the config
JsonNode has the os
and architecture
properties, which we should be able to use to answer this question appropriately.
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.
Changed it to use the os
of the base image config.
7e04d74
to
fe559a7
Compare
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 @Danielku15 - this LGTM now. This is a great piece of work, I really appreciate you taking the time to dig in and solve this.
@Danielku15 can you resolve conflicts with main? |
# Conflicts: # Microsoft.NET.Build.Containers/ImageConfig.cs # Microsoft.NET.Build.Containers/Layer.cs
Tested this this morning and it works great - we'll need to do some work writing tests that will work on a windows-configured daemon (our default local registry doesn't work, for example), but the core functionality is great. |
Related Issue: #117
Key change in this PR are:
Files/
andHives/
and not having a drive letter folder.BUILTIN\Users
access to the app folder.EnsureDirectoryEntries
into a local function.