-
Couldn't load subscription status.
- Fork 713
Add ContainerBuildOptions support to ResourceContainerImageBuilder for customizing dotnet publish #10074
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
Conversation
dotnet publish built imagesThere 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.
Update the implementation to respect the ImageFormat and TargetPlatform in the DockerContainerRuntime and PodmanContainerRuntime.
| } | ||
|
|
||
| // Backward compatibility methods | ||
| public Task BuildImagesAsync(IEnumerable<IResource> resources, CancellationToken cancellationToken) |
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.
Remove the backwards compatibility methods. This is an experimental type and backwards compatible is not a consideration.
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.
Removed the backward compatibility methods from ResourceContainerImageBuilder as requested. Since this is an experimental API, backward compatibility is not a consideration. Commit: 94d03a4
Updated the implementation to respect ImageFormat and TargetPlatform in both DockerContainerRuntime and PodmanContainerRuntime. The container runtimes now accept ContainerBuildOptions and apply the appropriate command-line arguments for platform, format, and output path support. Commit: 94d03a4 |
94d03a4 to
6c585a3
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.
@copilot Address the feedback. Make sure that any arguments passed to the commands from the ContainerBuildOptions are quoted correctly.
Fixed argument quoting for all ContainerBuildOptions commands in DockerContainerRuntime, PodmanContainerRuntime, and ResourceContainerImageBuilder. All file paths, image names, platform values, and MSBuild properties are now properly quoted. Commit: 471e770 |
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 More changes are required here.
Co-authored-by: captainsafia <[email protected]>
… backward compatibility methods Co-authored-by: captainsafia <[email protected]>
Co-authored-by: captainsafia <[email protected]>
…pings, enhance Docker/Podman runtime support Co-authored-by: captainsafia <[email protected]>
Co-authored-by: eerhardt <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| var spec = new ProcessSpec("docker") | ||
| { | ||
| Arguments = "info", | ||
| Arguments = "buildx version", |
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.
Why did this change from info to buildx version?
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're taking a dependency on buildx now to support all the output types and buildx is an add-on that's included by default in certain implementations. docker info isn't a sufficient enough check.
…set properties, and eliminate code duplication Co-authored-by: eerhardt <[email protected]>
|
|
||
| if (!string.IsNullOrEmpty(options?.OutputPath)) | ||
| { | ||
| outputType += $",dest=\"{options.OutputPath}/{imageName}.tar\""; |
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.
How does quotes inside of quotes work here?
--output "type=oci,dest="C:\temp\foo.tar""
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.
Quoted values have been working fine for me in Bash/zshell. Some further digging reveals that Powershell doesn't handle this as nicely so the best thing to do is no quotes, although that assumes an output path with no spaces. :/
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.
Why can't this be inverted? Quote the path, but don't quote around the value to --output?
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.
You mean like --output type=oci,dest="C:\temp\foo.tar"? I think that's probably worse because there is no indication to the shell that the value is on cohesive string other than the whitespace separation. We're also pretty consistently quoting outer arguments in our CLI calls.
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.
You mean like --output type=oci,dest="C:\temp\foo.tar"?
Yes
I think that's probably worse because there is no indication to the shell that the value is on cohesive string other than the whitespace separation.
What else could have whitespace?
| { | ||
| // Extract resource name from imageName for the file name | ||
| var resourceName = imageName.Split('/').Last().Split(':').First(); | ||
| arguments += $" --output \"{options.OutputPath}/{resourceName}.tar\""; |
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.
Path.Combine
|
|
||
| if (!string.IsNullOrEmpty(options?.OutputPath)) | ||
| { | ||
| outputType += $",dest={Path.Combine(options.OutputPath, imageName)}.tar"; |
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.
There is a difference in behavior between the .NET Project publish and this. The .NET Project publish appears to write .tar.gz file extensions, where this does .tar.
How is a caller supposed to know the name of the files that were outputted have different name formats? Should they assume projects product .tar.gz and docker files produce .tar?
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'm not sure if .NET publish actually gzips the tarball after it creates it but docker build/podman build don't provide this as an option. We could change the filenames but that feels misleading given they don't actually produce gzipped assets. Maybe we just document?
|
/backport to release/9.4 |
|
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16198906310 |
This PR adds support for passing additional arguments to
dotnet publishcommands when building container images through theResourceContainerImageBuilder.Problem
The
ResourceContainerImageBuildercurrently invokesdotnet publishwith hardcoded arguments and doesn't support setting additional MSBuild properties that callers might need, such as:/p:ContainerImageFormat/p:ContainerArchiveOutputPath/p:ContainerRuntimeIdentifierSolution
Added a strongly-typed API through new types:
New API Surface
Updated
IResourceContainerImageBuilderinterface:Usage Example
This generates the command:
Implementation Details
Testing
ContainerBuildOptionsfunctionalityFixes #10000.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.