-
-
Notifications
You must be signed in to change notification settings - Fork 335
Clean up WithResourceMapping #1497
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
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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 for taking care of this! Could you please remove/revert the unnecessary changes (like extra whitespace, line breaks, etc.)? That would make the review much easier 🙏.
8e1a8dc
to
0dbb5a1
Compare
0dbb5a1
to
3399e09
Compare
Sure, sorry for that. I also rebased while at it. |
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 for the PR 🙏.
After taking a closer look at the PR, I remembered why we didn't introduce this earlier.
The issue with FileSystemInfo
implementations (DirectoryInfo
, FileInfo
) is that they obviously refer to the underlying OS.
On Windows, some of the tests fail because files can't be copied into the container properly.
For example, using new FileInfo("/test.txt")
ends up referring to C:\test.txt
, which isn't what we want (as target).
We need an abstraction that works with the target container OS as well, unfortunately.
if (source.IsFile) | ||
{ | ||
return WithResourceMapping(new FileResourceMapping(source.AbsolutePath, target, fileMode)); | ||
return WithResourceMapping(new FileResourceMapping(source.AbsolutePath, target.FullName, fileMode)); |
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.
return WithResourceMapping(new FileResourceMapping(source.AbsolutePath, target.FullName, fileMode)); | |
return WithResourceMapping(new FileInfo(source.AbsolutePath), target, fileMode); |
} | ||
else | ||
|
||
return WithResourceMapping(new UriResourceMapping(source, target.FullName, fileMode)); |
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.
I believe this will copy the file to the wrong place.
using DotNet.Testcontainers.Tests.Fixtures; | ||
using Xunit; | ||
|
||
public sealed class WithResourceMappingTest(AlpineBuilderFixture image) : IClassFixture<AlpineBuilderFixture> |
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.
Many of the methods are tested here (this supports HTTP too):
testcontainers-dotnet/tests/Testcontainers.Platform.Linux.Tests/TarOutputMemoryStreamTest.cs
Line 118 in 6c5b160
public async Task TestFileExistsInContainer() |
Pity. What about providing our own abstraction for File- vs. Directory-Paths? Basically a Record containing a single String. I imagine two classes with each a static helper like |
Sounds like a good idea 👍. We probably just need something like this (and the equivalent for directory): public readonly record struct FilePath
{
private FilePath(string value)
{
Value = Unix.Instance.NormalizePath(value); // This method already exists in Testcontainers.
}
public string Value { get; }
public static FilePath Of(string path) => new FilePath(path);
} WDYT? |
4900ecd
to
8fa5f1b
Compare
What does this PR do?
Overhaul of
WithResourceMapping()
.The methods with string arguments are deprecated and replaced with the following methods. These signatures are explicit with regard to the target path being a file or directory path.
Why is it important?
With the previous API, it wasn't consistent whether the target string path is a directory or a file.
Related issues