-
Notifications
You must be signed in to change notification settings - Fork 744
Replace bind mount sources with environment placeholders in Docker Compose publishing #13223
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: davidfowl <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13223Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13223" |
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.
Pull request overview
This PR enhances Docker Compose publishing to improve portability by replacing bind mount source paths with environment variable placeholders. Instead of hardcoding host-specific paths directly in the docker-compose.yaml file, the implementation now uses environment variable placeholders (e.g., ${MY_CONTAINER_BINDMOUNT_0}) with the actual paths stored in the .env file. This allows developers to customize paths for their specific machines while keeping the compose file portable.
Key changes:
- Bind mount sources replaced with environment placeholders named
{RESOURCE_NAME}_BINDMOUNT_{INDEX} - Docker socket paths (
/var/run/docker.sock,\\.\pipe\docker_engine) preserved as-is for portability - Warning message logged when bind mounts are encountered to inform users of portability concerns
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting.Docker/DockerComposeEnvironmentContext.cs |
Core implementation that creates environment placeholders for bind mount sources and skips Docker socket paths |
src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs |
Fixed .env file generation to include default values for captured environment variables |
src/Aspire.Hosting.Docker/DockerComposePublisherLoggerExtensions.cs |
Added warning log message for bind mounts with host-specific paths |
tests/Aspire.Hosting.Docker.Tests/DockerComposePublisherTests.cs |
Added three new tests covering bind mount placeholder replacement, Docker socket preservation, and mixed scenarios |
tests/Aspire.Hosting.Docker.Tests/Snapshots/* |
Updated snapshot files verifying correct placeholder generation in yaml and default values in .env files |
| Assert.DoesNotContain("OLD_STAGING_KEY", envFileContent); | ||
| } | ||
|
|
||
| [Fact] |
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.
@copilot add a test with a windows specific absoulte path specified for the bind mount source (that only runs on windows).
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.
Added test PublishAsync_WindowsAbsoluteBindMountPath_ReplacedWithEnvironmentPlaceholders that uses Windows-specific paths like C:\Users\TestUser\Data and skips on non-Windows platforms using Assert.Skip(). Fixed in 2580b5f.
…ws-specific test Co-authored-by: davidfowl <[email protected]>
|
@copilot generate snapshots for the new test. |
Co-authored-by: davidfowl <[email protected]>
|
@copilot We need to make 2 changes:
This should allow for mutating the full set of generated content programmatically |
…k for programmatic mutation of env vars Co-authored-by: davidfowl <[email protected]>
Implemented all three changes in 227dbfd:
Added a test demonstrating how to use |
|
@copilot The name on CapturedEnvironmentVariable needs to be readonly. Changing it can mess up the references in the docker compose file. We also need IResource? on CapturedEnvironmentVariable because sometimes the source might be an object that has no resouce reference but might be an annotation on a resource. We want to be able to tell which resource if any that this environment variable is associated with. Add a test that looks something like: // Mount certs directory This sample needs to be able to remove the generated placeholder the default logic added |
…moving generated placeholders Co-authored-by: davidfowl <[email protected]>
Implemented in 15a57ab:
|
Description
Bind mounts in Docker Compose publishing now use environment variable placeholders instead of hardcoded host paths, improving portability across machines.
Changes:
{RESOURCE_NAME}_BINDMOUNT_{INDEX}(e.g.,MY_CONTAINER_BINDMOUNT_0).envfile as default values/var/run/docker.sock,\\.\pipe\docker_engine) preserved as-is.envfile generation to include default values for all captured environment variablesC:\Users\TestUser\Data) with snapshot filesCapturedEnvironmentVariabletype to replace tuple inCapturedEnvironmentVariablesfor better API surface:Nameproperty is readonly to prevent breaking compose file referencesIResource?property to track which resource the environment variable is associated withSourceproperty to hold the originating object (e.g.,ContainerMountAnnotation)ContainerMountAnnotationand associated resource when adding bind mount environment variablesConfigureEnvironmentextension method that allows programmatic mutation of captured environment variables before they are written to the .env file (similar toConfigureComposeFile)Example output:
docker-compose.yaml:
.env:
Example using ConfigureEnvironment to modify paths:
Example using ConfigureEnvironment to remove generated placeholders:
Fixes #9055
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplateOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.