-
Notifications
You must be signed in to change notification settings - Fork 750
Display resource endpoints for Docker Compose deploy #13216
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,19 +1,59 @@ | ||||||||||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||||||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||||||||||
|
|
||||||||||||||
| #pragma warning disable ASPIREPIPELINES001 | ||||||||||||||
|
|
||||||||||||||
| using System.Globalization; | ||||||||||||||
| using System.Text; | ||||||||||||||
| using System.Text.Json; | ||||||||||||||
| using System.Text.Json.Serialization; | ||||||||||||||
| using Aspire.Hosting.ApplicationModel; | ||||||||||||||
| using Aspire.Hosting.Dcp.Process; | ||||||||||||||
| using Aspire.Hosting.Docker.Resources.ComposeNodes; | ||||||||||||||
| using Aspire.Hosting.Docker.Resources.ServiceNodes; | ||||||||||||||
| using Aspire.Hosting.Pipelines; | ||||||||||||||
| using Aspire.Hosting.Utils; | ||||||||||||||
| using Microsoft.Extensions.Logging; | ||||||||||||||
|
|
||||||||||||||
| namespace Aspire.Hosting.Docker; | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// Represents a compute resource for Docker Compose with strongly-typed properties. | ||||||||||||||
| /// </summary> | ||||||||||||||
| public class DockerComposeServiceResource(string name, IResource resource, DockerComposeEnvironmentResource composeEnvironmentResource) : Resource(name), IResourceWithParent<DockerComposeEnvironmentResource> | ||||||||||||||
| public class DockerComposeServiceResource : Resource, IResourceWithParent<DockerComposeEnvironmentResource> | ||||||||||||||
| { | ||||||||||||||
| private readonly IResource _targetResource; | ||||||||||||||
| private readonly DockerComposeEnvironmentResource _composeEnvironmentResource; | ||||||||||||||
|
|
||||||||||||||
| /// <summary> | ||||||||||||||
| /// Initializes a new instance of the <see cref="DockerComposeServiceResource"/> class. | ||||||||||||||
| /// </summary> | ||||||||||||||
| /// <param name="name">The name of the resource.</param> | ||||||||||||||
| /// <param name="resource">The target resource.</param> | ||||||||||||||
| /// <param name="composeEnvironmentResource">The Docker Compose environment resource.</param> | ||||||||||||||
| public DockerComposeServiceResource(string name, IResource resource, DockerComposeEnvironmentResource composeEnvironmentResource) : base(name) | ||||||||||||||
| { | ||||||||||||||
| _targetResource = resource; | ||||||||||||||
| _composeEnvironmentResource = composeEnvironmentResource; | ||||||||||||||
|
|
||||||||||||||
| // Add pipeline step annotation to display endpoints after deployment | ||||||||||||||
| Annotations.Add(new PipelineStepAnnotation((factoryContext) => | ||||||||||||||
| { | ||||||||||||||
| var steps = new List<PipelineStep>(); | ||||||||||||||
|
|
||||||||||||||
| var printResourceSummary = new PipelineStep | ||||||||||||||
| { | ||||||||||||||
| Name = $"print-{resource.Name}-summary", | ||||||||||||||
|
||||||||||||||
| Name = $"print-{resource.Name}-summary", | |
| Name = $"print-{_targetResource.Name}-summary", |
Outdated
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.
| Action = async ctx => await PrintEndpointsAsync(ctx, composeEnvironmentResource).ConfigureAwait(false), | |
| Action = async ctx => await PrintEndpointsAsync(ctx, _composeEnvironmentResource).ConfigureAwait(false), |
Copilot
AI
Nov 26, 2025
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.
Missing blank line before the XML documentation comment. According to the codebase formatting conventions, there should be a blank line between the closing brace of the constructor and the XML documentation comment for the next member.
| } | |
| } |
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.
Doesn't this need to handle potential slow startup of services? What guaranteees that docker compose ps --format json will have the information by the time you call it?
Copilot
AI
Nov 26, 2025
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.
The PrintEndpointsAsync method is missing XML documentation. According to the Aspire XML documentation guidelines, internal methods should have brief <summary> tags explaining what they do. Consider adding:
/// <summary>
/// Prints the endpoints for the Docker Compose service after deployment.
/// </summary>| private async Task PrintEndpointsAsync(PipelineStepContext context, DockerComposeEnvironmentResource environment) | |
| { | |
| /// <summary> | |
| /// Prints the endpoints for the Docker Compose service after deployment. | |
| /// </summary> | |
| private async Task PrintEndpointsAsync(PipelineStepContext context, DockerComposeEnvironmentResource environment) |
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 this declaration and check for dockerComposeFilePath since we do it in DockerComposeEnvironmentResource.GetDockerComposeArguments now.
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 comment that shows an example of the expected format being parsed here from docker compose.
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 comment that shows an example of the expected format being parsed here from docker compose.
Copilot
AI
Nov 26, 2025
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.
The logic for determining which endpoints to display could be clearer. The condition endpointMapping.IsExternal || scheme is "http" or "https" is confusing because:
externalEndpointMappingson line 371 already filters to external endpoints- When
FirstOrDefaultreturns no match (line 410),endpointMappingwill be the default struct value whereIsExternal = false - The fallback to checking
scheme is "http" or "https"suggests the intent is to show all http/https ports even without explicit mapping
Consider adding a comment to clarify this logic, or restructuring to make the intent clearer:
// Show endpoint if: it matches an external endpoint mapping, OR it's an http/https port (published ports are external by default)
var hasExplicitMapping = endpointMapping.Resource is not null;
if (hasExplicitMapping || scheme is "http" or "https")
{
var endpoint = $"{scheme}://localhost:{publisher.PublishedPort}";
endpoints.Add(endpoint);
}
Copilot
AI
Nov 26, 2025
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.
The new endpoint printing functionality introduced in the PrintEndpointsAsync method and the pipeline step configuration lacks test coverage. Consider adding tests to verify:
- Endpoint discovery and display when containers are running
- Behavior when no external endpoints are configured
- Handling of multiple endpoints with different schemes
- Error handling when Docker Compose commands fail
The test file tests/Aspire.Hosting.Docker.Tests/DockerComposePublisherTests.cs has comprehensive test coverage for other Docker Compose functionality and would be an appropriate location for these tests.
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.