Make sure bind mount paths are relative to the app host directory #3467
Conversation
- Made it so that bind mounts annotations are resolved relative to the app host directory by default. - Bind mounts without a rooted path will throw an error. - Removed logic from ApplicationExecutor that checks the annotation data since this is checked in the constructor. - Added tests (moved tests to Containers folder)
dotnet-issue-labeler
bot
added
the
area-app-model
|
Looks pretty good. Just one comment about hard coding paths. |
| @@ -44,8 +44,10 @@ public void AddNatsContainerWithDefaultsAddsAnnotationMetadata() | |||
| [Fact] | |||
| public void AddNatsContainerAddsAnnotationMetadata() | |||
| { | |||
| var path = OperatingSystem.IsWindows() ? @"C:\tmp\dev-data" : "/tmp/dev-data"; | |||
There was a problem hiding this comment.
Probably shouldn't hard code these paths.
There was a problem hiding this comment.
Why not? We need a well-formed path because we call GetFullPath on the input. This makes the assert a little more sane 😄
| internal string? ProjectDirectory => _projectDirectory.Value; | ||
| internal string? ProjectDirectory | ||
| { | ||
| get => _projectDirectory ?? _projectDirectoryLazy.Value; |
There was a problem hiding this comment.
| get => _projectDirectory ?? _projectDirectoryLazy.Value; | |
| get => _projectDirectory ??= _projectDirectoryLazy.Value; |
Might as well set this after you evaluate it, right?
Another thought is to remove the field altogether. I don't see it being used in tests. Am I missing something?
There was a problem hiding this comment.
Yea the property is being used in tests.
There was a problem hiding this comment.
It's confusing to mark the field with the comment
There was a problem hiding this comment.
What do you mean? This is only a field so we can set it.
There was a problem hiding this comment.
Typically when someone puts a comment like:
// for testing
internal Foo() {}It means Foo is getting called/used by tests.
A whole other option is to just use a single field, and when it is null during get generate the value.
There was a problem hiding this comment.
A whole other option is to just use a single field, and when it is null during get generate the value.
But I guess the lazy value could be null, so that helps little.
| } | ||
|
|
||
| [Fact] | ||
| public void EnsureContainerWithArgsEmitsContainerArgs() |
There was a problem hiding this comment.
Why did this test get deleted? I don't see it on the "right" side.
There was a problem hiding this comment.
My goal is to move all tests (incrementally) to resource specific folders (projects). This manifest generation test needs to die 😄 (or only be used for testing cross cutting things like writing the entire manifest).
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8607190574 |
Fixes #3323
Microsoft Reviewers: Open in CodeFlow