Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix tests on windows and add workflow #143

Closed
wants to merge 7 commits into from
Closed

Fix tests on windows and add workflow #143

wants to merge 7 commits into from

Conversation

gabriel-samfira
Copy link
Contributor

@gabriel-samfira gabriel-samfira commented Nov 17, 2022 •
edited
Loading

This change adds a new workflow to test this project on Windows. It also fixes tests and some functionality.

  • Path separator is converted to whatever is specific to the OS we are running on
  • containerd/continuity package updated to latest commit. In newer versions of continuity, ErrNotSupported is ignored when resolving xattrs, which is unsupported on Windows.
  • UIDs and GIDs are ignored on Windows
  • Add workflows for Windows
  • Disable some tests on Windows

Fixes: #142

@gabriel-samfira gabriel-samfira marked this pull request as draft November 18, 2022 15:22
@gabriel-samfira
Copy link
Contributor Author

Noticed a couple of bugs. Fixing and committing soon.

@gabriel-samfira gabriel-samfira marked this pull request as ready for review November 18, 2022 21:05
@gabriel-samfira
Fix copyFileInfo on Windows
e66cb41 
The c.chowner function was being ignored. This change makes use of the
supplied chowner and calls Chown() on the file. If no chowner is
specified, we copy over the DACL and owner information from the source.
@gabriel-samfira
Copy link
Contributor Author

This is ready for review when you get a chance. The bug I noticed was in copyFileInfo(). I had forgotten to use the chowner when defined and kept copying the original file ownership information.

As a side note, I have been investigating why ownership information doesn't carry from layer to layer. Only DACLs seem to carry over. This seems to be a limitations in HCS, not in any library or application code, and happens when layers get activated.

It's worth having this code added, however, as once that behavior is fixed in HCS, we would already have coverage in buildkit/containerd/moby. Also, DACLs seem to be retained, which should allow users to retain whatever access they had to the file regardless of who owns it.

@gabriel-samfira
Copy link
Contributor Author

Hi!

Does anyone have some time to review these changes? 😄

tonistiigi
tonistiigi reviewed Jan 9, 2023
View reviewed changes
Copy link
Owner

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't have notifications set up for this repo.

Left a couple of quick comments. If the path conversions are needed in existing files we might need to do it in separate commits or prs so they can be evaluated more clearly if the use-case requires normalized path or current system path.

diskwriter.go Outdated Show resolved Hide resolved
diff.go Outdated Show resolved Hide resolved
@tonistiigi
Copy link
Owner

Feel free to move the functionally separate parts to smaller PRs for easier review.

tonistiigi
tonistiigi reviewed Jan 9, 2023
View reviewed changes
receive_test.go Outdated Show resolved Hide resolved
@gabriel-samfira gabriel-samfira mentioned this pull request Jan 18, 2023
Merged
@gabriel-samfira
Copy link
Contributor Author

Feel free to move the functionally separate parts to smaller PRs for easier review.

Splitting this up in multiple PRs today. Thanks for the review, and apologies about the delay!

@gabriel-samfira
Undo dir mod time removal
4594477 
Mistakenly removed those tests and the mod time code.

Signed-off-by: Gabriel Adrian Samfira <[email protected]>
@gabriel-samfira gabriel-samfira mentioned this pull request Feb 9, 2023
Merged
@gabriel-samfira
Copy link
Contributor Author

I opened #145 #146 #147 #148 and #149 to replace this PR. Closing this one. FreeBSD tests seem to fail at setting up vagrant. Probably a github issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Windows CI workflow
2 participants
@gabriel-samfira @tonistiigi