-
Couldn't load subscription status.
- Fork 712
Custom URLs improvements #8743
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
Custom URLs improvements #8743
Conversation
| /// <remarks> | ||
| /// Set this to <see langword="false"/> to only show this URL in the resource details grid. | ||
| /// </remarks> | ||
| public bool ShowOnResourcesPage { get; set; } = true; |
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.
Thoughts on the property name? This maps to the IsInternal property on the URL snapshot and resource server APIs but that name is pretty terrible so trying to make it more descriptive here.
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.
make it an enum?
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.
Ha I actually started that way, a flags enum.
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.
Actually not flags as the resource snapshots, dashboard, and resource server protocol currently only support the boolean state (IsInternal). So perhaps something like the following, although I admit I don't love it:
/// <summary>
/// Specifies where the URL should be displayed.
/// </summary>
public enum UrlDisplayLocation
{
/// <summary>
/// Show the URL in locations where either the resource summary or resource details are being displayed.
/// </summary>
SummaryAndDetails,
/// <summary>
/// Show the URL in locations where the full details of the resource are being displayed.
/// </summary>
DetailsOnly
}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.
Flags
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.
And modify the protocol?
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.
Maybe flags is wrong anyways because alll combinations aren’t valid so the above is fine?
| internal ResourceUrlAnnotation WithEndpoint(EndpointReference endpoint) | ||
| { | ||
| return new() | ||
| { | ||
| Url = Url, | ||
| DisplayText = DisplayText, | ||
| Endpoint = endpoint, | ||
| DisplayOrder = DisplayOrder, | ||
| ShowOnResourcesPage = ShowOnResourcesPage | ||
| }; | ||
| } |
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.
This was added to support the new overload of WithUrlForEndpoint that allows adding a new URL for an endpoint. The callback in that case has the user return the ResourceUrlAnnotation and then we call this method to ensure the correct endpoint is associated with it. The user could set it themselves in the callback as we pass them the EndpointReference, but this way ensures it's always correct. The Endpoint property is currently marked init and it didn't seem to me that anything else warranted changing it to set but I could be convinced otherwise.
| public EndpointReference? GetEndpoint(string name) => | ||
| Resource switch | ||
| { | ||
| IResourceWithEndpoints resourceWithEndpoints => resourceWithEndpoints.GetEndpoint(name), | ||
| _ => null | ||
| }; |
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.
Makes it easy for callers of WithUrls to get an endpoint on the resource without needing to do type checking/casting to IResourceWithEndpoints.
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.
Should it throw instead of returning null?
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.
Throwing is a bit mean how do you avoid throwing?
| public static IResourceBuilder<T> WithUrlForEndpoint<T>(this IResourceBuilder<T> builder, string endpointName, Func<EndpointReference, ResourceUrlAnnotation> callback) | ||
| where T : IResourceWithEndpoints | ||
| { | ||
| builder.WithUrls(context => | ||
| { | ||
| var endpoint = builder.GetEndpoint(endpointName); | ||
| if (endpoint.Exists) | ||
| { | ||
| var url = callback(endpoint).WithEndpoint(endpoint); | ||
| context.Urls.Add(url); | ||
| } | ||
| else | ||
| { | ||
| context.Logger.LogWarning("Could not execute callback to add an endpoint URL as no endpoint with name '{EndpointName}' could be found on resource '{ResourceName}'.", endpointName, builder.Resource.Name); | ||
| } | ||
| }); | ||
|
|
||
| return builder; | ||
| } |
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.
This new overload can be used to add a URL for an endpoint, whereas the existing overload is used to update the (first) URL for an endpoint.
|
This looks good to me. Just one comment about throwing vs. returning null. Should we ever get into the situation where that resource is no |
Yes, because you can add URLs to any resource |
|
LLMs can’t parse the comma!?😅 |
|
Looks like I'm late to the party. 😄 When will this be available? |
|
9.3 |
When will that be available? Any previews, in the meantime? |
|
There’s always daily builds https://github.com/dotnet/aspire/blob/main/docs/using-latest-daily.md |
|
@DamianEdwards, does this also add the possibility for the display text for the URL to be only on the summary but not on the details? The way I see it, on the summary view, the text should be the display name. On the details view, it should show the Link, the Display name and the Endpoint name. The configuration of the endpoint should assume the Display name as the Endpoint name by default. |
That's what it does now. |
"now" == 9.3.0 ? |
|
@paulomorgado yes |



Description
Bunch of changes to resource URL customization based on user feedback in 9.2:
ResourceUrlsCallbackContextWithUrlForEndpointfor adding extra endpoint URLsFixes #8725, #8636, #8640, #8638, #6454
Checklist
<remarks />and<code />elements on your triple slash comments?Image shows the Frontend resource in TestShop only showing one URL in the Resources table but all URLs in the details view. The details view has been updated to show three columns so that the URL address, text, and endpoint are now clear.
Here's an example of the details view for a resource with a non-HTTP URL: