Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Nest sidecar type resources by default in Dashboard #7259

Open
1 task done
afscrome opened this issue Jan 26, 2025 · 10 comments · May be fixed by #7337
Open
1 task done

Nest sidecar type resources by default in Dashboard #7259

afscrome opened this issue Jan 26, 2025 · 10 comments · May be fixed by #7337
Assignees
@eerhardt
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Milestone
9.1

Comments

@afscrome
Copy link
Contributor

afscrome commented Jan 26, 2025 •
edited
Loading

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

9.1's dashboard is adding support for showing parent / child resources as nested in the UI. This works by default for IResourceWithParent implementations (such as databases), but there are other instances where this could also be done - particularly for sidecar type containers such as redis insights, redis commander, pg admin etc.

Image

As well as built in resources, I'd also like to be able to specify this on my own executables and containers - e.g. a migrator executable resource that runs dotnet ef update.

Describe the solution you'd like

I'd like to see an API on IResourceBuilder which I can use to set the parent - e.g. migrator.WithParent(database), and to have this call set on appropriate resources built in to Aspire.

Additional context

<PackageReference Include="Aspire.Hosting.AppHost" Version="9.1.0-preview.1.25075.3" />

I have been able to make this somewhat work with the following code, but it's very temperamental - only works about roughly 10% of the time - trying to publish resource notifications for aspire managed resources seems to be fighting with Aspire's built in orchestration.

builder.Eventing.Subscribe<BeforeResourceStartedEvent>(migrator.Resource, async (evt, ct) =>
{
    var resourceNotificationService = evt.Services.GetRequiredService<ResourceNotificationService>();

    await resourceNotificationService.PublishUpdateAsync(migrator.Resource, x => x with
    {
        Properties = [
            ..x.Properties,
            new("resource.parentName", sql.Resource.Name)
            ]
    });
});
@davidfowl
Copy link
Member

These resources aren’t currently not sidecars as they show all databases.

@afscrome
Copy link
Contributor Author

afscrome commented Jan 27, 2025 •
edited
Loading

Oh I think I see what you're saying - even if there are multiple redis instance, there's still only a single commander / insights, so a direct parent/child relation doesn't really work.

var redis = builder.AddRedis("redis").WithRedisCommander().WithRedisInsight();
var redis2 = builder.AddRedis("redis2").WithRedisCommander().WithRedisInsight();

Image

Even if not applicable to the core resources suggested above, I'd still like an API so I can apply this to my own resources.

@davidfowl
Copy link
Member

Agree. @JamesNK it seems there’s no way outside of IResoueceWithParent

@davidfowl davidfowl added this to the 9.1 milestone Jan 27, 2025
@davidfowl davidfowl added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 27, 2025
@JamesNK
Copy link
Member

JamesNK commented Jan 28, 2025

What's the change you want? An annotation+method for nesting a resource as parent/child?

@eerhardt eerhardt self-assigned this Jan 29, 2025
@eerhardt
Copy link
Member

What's the change you want? An annotation+method for nesting a resource as parent/child?

I will try coming up with a PR for this approach.

@eerhardt
Copy link
Member

It looks like there already is

aspire/src/Aspire.Hosting/ApplicationModel/ResourceRelationshipAnnotation.cs

Lines 8 to 23 in 0dce648

/// <summary>
/// An annotation which represents the relationship between two resources.
/// </summary>
[DebuggerDisplay("Type = {GetType().Name,nq}, Resource = {Resource.Name}, RelationshipType = {Type}")]
public sealed class ResourceRelationshipAnnotation(IResource resource, string type) : IResourceAnnotation
{
/// <summary>
/// The resource that the relationship is to.
/// </summary>
public IResource Resource { get; } = resource;
/// <summary>
/// The relationship type.
/// </summary>
public string Type { get; } = type;
}

Adding one of these with Type = Parent should do work, but currently doesn't. We can make an easy API for adding these annotations. And then fixing the code to respect those annotations as well as IResourceWithParent.

@JamesNK
Copy link
Member

JamesNK commented Jan 30, 2025

Lots of validation will be needed here to make sure:

  • A resource doesn't have multiple parents
  • Resources don't have a circular parent relationship

An error needs to happen and it is better it happens when the app model is built and run. We want to avoid a the dashboard erroring when it tries to render something that's impossible.

eerhardt added a commit to eerhardt/aspire that referenced this issue Jan 30, 2025
@eerhardt
Initial change
c7745e6 
Fix dotnet#7259
@eerhardt eerhardt linked a pull request Jan 30, 2025 that will close this issue
Draft
18 tasks
@eerhardt
Copy link
Member

A resource doesn't have multiple parents

Should this be an error, or should we say "Last one wins" like we do for other annotations?

@davidfowl
Copy link
Member

Last wins sounds better

@davidfowl
Copy link
Member

@eerhardt as part of this change, use the attribute in a various resources (make an assesment of the ones where it makes sense to do so).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eerhardt eerhardt

Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Milestone
9.1
Development

Successfully merging a pull request may close this issue.

Add the ability to set parent relationships in the dashboard eerhardt/aspire
4 participants
@davidfowl @afscrome @JamesNK @eerhardt