Skip to content

Conversation

@DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Mar 21, 2025

Description

This change introduces support for resource URLs. Resource endpoints get automatically projected into URLs, which can be customized, and extra URLs can be added that aren't associated with endpoints at all.

Fixes #6457
Fixes #7104
Fixes #4319
Contributes to #5209

Screen.Recording.2025-03-21.125917.mp4

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
  • Does the change make any security assumptions or guarantees?
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Is this introducing a breaking change?
        • No
      • Link to aspire-docs issue (please use this doc-idea template): [pending]

@DamianEdwards
Copy link
Member Author

DamianEdwards commented Mar 21, 2025

@JamesNK right now the dashboard is not showing URLs that aren't associated with an endpoint in the resource details grid as there's currently a requirement for property grid items to have a name as that's used as the key. @davidfowl is suggesting we change the current "Endpoints" grid to instead just show URLs, and re-think the columns to match, but we'll need to figure out how to handle items with no name. Would appreciate your help there. I guess we might want to revisit the column header in the Resources page too, i.e. change "Endpoints" to "URLs".

@JamesNK
Copy link
Member

JamesNK commented Mar 21, 2025

I think we change "Endpoints" to "Urls" everywhere.

Edit: However, there can be TCP endpoints which aren't Urls. It would be a little weird having a column called "Urls" that then contains values like tcp://localhost:8080. Technically they're URIs.

image

In the resource details we could change the columns to:

"Link" and "Endpoint name"

  • "Link" would be a hyperlink with the display name as the text, and fallback to the URL if there is no display name.
  • "Endpoint name" would be the optional endpoint name. If there is no endpoint name then the table cell would display "-" like other columns.

we'll need to figure out how to handle items with no name

Anything can be the row key, as long as it is unique. I'm guessing it is possible to have the same URL multiple times so that couldn't be the key. There wasn't a good key for volume so we used the index of the volume instance in the collection as the key.

object IPropertyGridItem.Key => index;

I think you could do the same thing here.

Comment on lines 91 to 96
<AspireTemplateColumn Sortable="true" SortBy="@(GridSort<DisplayedEndpoint>.ByAscending(i => i.DisplayName))" Title="@ControlStringsLoc[nameof(ControlsStrings.DisplayNameColumnHeader)]">
<GridValue ValueDescription="@ControlStringsLoc[nameof(ControlsStrings.DisplayNameColumnHeader)]"
Value="@context.DisplayName"
EnableHighlighting="@(!string.IsNullOrEmpty(_filter))"
HighlightText="@_filter" />
</AspireTemplateColumn>
Copy link
Member

@JamesNK JamesNK Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of having the URL and display name as separate columns. I'd rather there was just a "Link" column which displayed a hyperlink. If there is a display name then it is the text for the link. If there isn't then use the URL.

Opening the text visualizer or copy would use the URL value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but there could also be values that aren't links, e.g. tcp://localhost:8080. Maybe link isn't the right header

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so we're saying back to two columns. "Endpoint name" is one of them and I think that's clear. The second one will contain the hyperlink if it's a URL, otherwise it will just be the URI text. Do we simply give it a combo name like "URI / Link"? Or should we just settle on "Link" and not worry too much about the non-hyperlinked URI case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards the latter option

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamint just go with "Link"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with "Link", take a look in the video in the PR description.

var url = context.Urls.FirstOrDefault(u => u.Endpoint?.EndpointName == endpointName);
if (url is not null)
{
url.DisplayText = Resources.DataExplorerUrlText;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work, because the requesting browser may be using a different locale than hosting. I think it should go in the place I linked before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. We need this to be localized in the dashboard itself then, as a known string?

Copy link
Member

@adamint adamint Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'm concerned just using the endpoint name will be too broad, we could use a combo of resource type + endpoint name? But since AzureCosmosDBEmulatorResource is just a container resource.. we can't use the resource type. Am I missing something or do we need to add a more specific identifier too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK rigged it up.

@JamesNK
Copy link
Member

JamesNK commented Mar 21, 2025

I think the link column should come first:

image

The optional Endpoint name first feels wierd. Especially since the grid is no longer focused on endpoints.

// The name of the url
string name = 1;
// The name of endpoint associated with the url
optional string endpoint_name = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this change to the proto is binary compatible. It's ok to make

@JamesNK
Copy link
Member

JamesNK commented Mar 21, 2025

Two column widths in URLs grid isn't consistent with others:

image

It should be consistent.

@JamesNK
Copy link
Member

JamesNK commented Mar 21, 2025

@mitchdenny @davidfowl There is a lot in this PR. I'm focusing on dashboard changes and taking a brief look at hosting changes.

It would be good for hosting experts (ya'll) to double check hosting changes.

@DamianEdwards
Copy link
Member Author

@JamesNK

Two column widths in URLs grid isn't consistent with others:

Given the column order will be reversed so the link is first and the endpoint name second, and I'd expect the link to be longer than the endpoint name in most cases, do you still think it makes sense to have the column widths match that of a standard name/value property grid?

@JamesNK
Copy link
Member

JamesNK commented Mar 21, 2025

I don't think either of them will be that long. A good URL display name will be short so it fits in the main grid so it should fit here. And most values here will still be the underlying URL anyway.

I think the consisency of grid widths, i.e. looking good, trumps showing a bit more of very long display names IMO.

@DamianEdwards
Copy link
Member Author

I think the consisency of grid widths, i.e. looking good, trumps showing a bit more of very long display names IMO.

I'm not sure I agree but I'm fine with making them match until such time that we see evidence of it being a problem.

var text = endpoint?.Url;
if (string.IsNullOrEmpty(text))
{
return "No endpoints";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say "No URLs". And while you're here, could you move the text to a resource file. I should have done that 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the resource graph only looks at endpoint-associated URLs right now. When it was showing all URLs it would pick the first one and then show the display text un-hyperlinked which wasn't useful at all. Having it show the actual endpoint address is more useful IMO.

@DamianEdwards DamianEdwards marked this pull request as ready for review March 21, 2025 23:55
var url = context.Urls.FirstOrDefault(u => string.Equals(u.Endpoint?.EndpointName, endpoint.Name, StringComparisons.EndpointAnnotationName));
if (url is not null)
{
if (launchUri.IsAbsoluteUri)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up: Discuss if an absolute launch urls wins over all over urls and becomes the only ones that shows up.


internal static class KnownUrls
{
public static class DataExplorer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most .NET thing ever. Why is this shared it's cosmos specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cos localization has to happen in the context of the dashboard. We do this for known commands too. We can add stuff to this for any other known link text we want to localize.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamianEdwards do a follow up and fix the AzureProvisioner to use this for deployments

@davidfowl davidfowl merged commit a8e7db7 into main Mar 22, 2025
163 checks passed
@davidfowl davidfowl deleted the damianedwards/endpoint-display branch March 22, 2025 04:13
@DamianEdwards
Copy link
Member Author

DamianEdwards commented Mar 22, 2025

do a follow up and fix the AzureProvisioner to use this for deployments

@davidfowl can you elaborate? Which URLs should the provisioner use exactly and for what scenarios? Maybe put details in an issue?

@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

5 participants