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.

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

Display nested resources in the dashboard #6589

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 2, 2024

Description

Automatically nested and collapse child resources that implement IResourceWithParent. By default, nested resources are collapsed. Parent information is sent in a resource property called resource.parentName. That means no proto changes required.

In test shop playground, catelogdb database resource is a child of postgres container resource:

resources-nested

Not 100% done yet, need to move some code to better places, add tests, tweak margins and paddings. But good enough to try out.

Edit: Updated screenshots

Resources page. Name column title and content is now aligned. I've consistently indented all content on the right by 8px across screens.

image

Trace detail page. Updated it to use button instead of +/- text. Consistent with resources page.

image

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?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

@davidfowl
Copy link
Member

This is cool, I think we need the annotation to do this for associated resources like pgadmin. That's currently not a child resource.

image

@JamesNK
Copy link
Member Author

JamesNK commented Nov 2, 2024

I wonder if we could do it with the relationship annotation. Special case "Parent" relationship and use it to drive nesting in UI.

I agree 100% it would be useful to nest more resources. I don't think we need to solve this problem in this PR. The focus here can be to get the basic UI right, and then we can iterate.

@davidfowl
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member

Real test failures

@DamianEdwards
Copy link
Member

This is cool, I think we need the annotation to do this for associated resources like pgadmin. That's currently not a child resource.

Yep, and doing it for replicas would be great too. Not sure whether it's better for the dashboard to synthesize a parent in that case, or whether it should be modelled somehow.

@Mrxx99
Copy link
Contributor

Mrxx99 commented Nov 3, 2024

This is cool, I think we need the annotation to do this for associated resources like pgadmin. That's currently not a child resource.

But that would not work if you have multiple postgres servers. Wouldn't they all use the same pgadmin instance? It wouldn't make sense then to nest it under only one of them. Or would it be listed mutiple times (under each of them)? Would that be confusing?

I think for those management UIs that can be used with multiple instances it would make sense to be able to manually define groups where the user can add all postgres instances and management UI (like pgadmin) inside. #6139

@Mrxx99
Copy link
Contributor

Mrxx99 commented Nov 3, 2024

If a child resource has a different status then healthy (but the parent is healthy), would that be noticeable if it is collapsed?
Maybe in collapsed state the parent could show a second status icon that with a symbol indicates it is for children, if children are unhealthy and show the most severe icon of all children?

@mitchdenny
Copy link
Member

@Mrxx99 brings up a good point. We really need Postgres to be easily accessible. One option might be taking the first step towards expanding the CommandExecuteResult protocol so that WithPgAdmin adds a ResourceCommandAnnotation and when executed it returns a URL that the dashboard would open.

This would be useful in a bunch of other scenarios as well. I don't think it needs to be in this PR now. Lets get this initial change in first and then we can look at the execute result protocol expansion (and then hide PGAdmin).

@mitchdenny
Copy link
Member

... if we took this approach we could completely hide the PGAdmin resource.

@Mrxx99
Copy link
Contributor

Mrxx99 commented Nov 3, 2024

... if we took this approach we could completely hide the PGAdmin resource.

I am not sure this would be great, as you won't see it's status or could easily see it's logs. Developers would wonder where it is, as it would not directly be visible. Why not just keep it a separate resource in the dashboard as it is currently, but adding commands to the postgres resource instances to open in pgamdin and/or pgweb (whichever resource is added)?
Would be great if pgadmin/pgweb support direct URL navigation to a specific server view.

@davidfowl
Copy link
Member

davidfowl commented Nov 3, 2024

We don't need to rathole on pgadmin here, @Mrxx99 is right and it shouldnt be a child because of how it works.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 4, 2024

Done. Review please 🙏

@davidfowl
Copy link
Member

Should parent resource be in the details view of the child resource as a known property?

image

@davidfowl
Copy link
Member

Also should there be a toggle to flatten the table again?

@davidfowl
Copy link
Member

cc @leslierichardson95

@DamianEdwards
Copy link
Member

Also should there be a toggle to flatten the table again?

Collapse all / Expand all, would be great.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

Should parent resource be in the details view of the child resource as a known property?

It's visible if you click show all. When relationships are added it will be visible there as well. I think what there is today is fine.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

Also should there be a toggle to flatten the table again?

Collapse all / Expand all, would be great.

Do you have example of a website that has UI that you'd like? I've been looking through websites like AzDo and GitHub and I can't find any examples of pages that have collapse/expand all.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

Should child resources be expanded or collapsed by default?

@DamianEdwards
Copy link
Member

Do you have example of a website that has UI that you'd like? I've been looking through websites like AzDo and GitHub and I can't find any examples of pages that have collapse/expand all.

None specific in mind, I actually had more traditional tree views in mind like the VS Solution Explorer. Definitely more a nice to have than required but I'll keep an eye out.

Should child resources be expanded or collapsed by default?

Collapsed.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

I want to leave expand/collapse all to another PR. You try nesting out without it and decide whether it is important and how you'd like it to be displayed.

@davidfowl
Copy link
Member

Did you test this with azure resources? Storage for e.g.?

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

Storage:

image

@davidfowl
Copy link
Member

Does it work when you're not running a container but instead use an azure resource? I see you handle custom resources but where does that happen outside of the app executor?

@JamesNK
Copy link
Member Author

JamesNK commented Nov 5, 2024

I think this really only works when the parent resource is a container. I added logic to set the parent property to existing logic in app executor for parent/children. That seems to only be implemented for containers.

@leslierichardson95
Copy link

Looking good! Thoughts on a few comments above:

  • +1 to Damian's comment on iterating on this with replicas later

  • I'm leaning toward resources being expanded by default from a discoverability standpoint. That said, if we're assuming that most users would know their app's resources and dependencies, I think we'd be ok collapsing by default. Would it be overkill to add a tooltip on the expand/collapse arrow indicating something like "view children"? 🤔

  • Maybe we could add "collapse all" and "expand all" headers near the filter icon? Semi-basing this off of the Azure config explorer:
    image

  • Other alternative: The VS Solution Explorer uses a collapse all/expand all icon that maybe we can consider repurposing here for additional consistency:
    image

@davidfowl
Copy link
Member

I think this really only works when the parent resource is a container. I added logic to set the parent property to existing logic in app executor for parent/children. That seems to only be implemented for containers.

We should fix this.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 6, 2024

We should fix this.

I don't think this needs to be improved in this PR. New issue: #6615

@Mrxx99
Copy link
Contributor

Mrxx99 commented Nov 6, 2024

We should fix this.

I don't think this needs to be improved in this PR. New issue: #6615

This new issue only targets projects and executables but AzureStorageResource (-> AzureProvisioningResource -> AzureBicepResource -> Resource) is neither.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 6, 2024

This already works when the parent resource is a container. AzureStorageResource is an example. There is a screenshot above of nested blob and queue resources.

However I see that AzureWebPubSubResource isn't a container, but can have a child AzureWebPubSubHubResource instances refer to it as a parent. That won't work automatically today. However, it looks like AzureWebPubSubHubResource isn't published so it isn't an issue.

@davidfowl What are your thoughts here? Should publishing a resource automatically set its parent ID property in ResourceNotificationService? Or is it the responsibility of the publisher? (DCP, AzureProvisioner, etc)

@davidfowl
Copy link
Member

However I see that AzureWebPubSubResource isn't a container, but can have a child AzureWebPubSubHubResource instances refer to it as a parent. That won't work automatically today. However, it looks like AzureWebPubSubHubResource isn't published so it isn't an issue.

This is a special case, the AzureWebPubSubHubResource doesn't get added to the model AFAIK

I am more concerned about things like azure storage.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 11, 2024

Can you give me an example of an app host which you're worried about not working?

@davidfowl
Copy link
Member

@JamesNK
Copy link
Member Author

JamesNK commented Nov 11, 2024

Take this sample main/playground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/Program.cs and uncomment RunAsEmulator

I tested and storage resources aren't nested.

The question is where should parent informaiton be set?

  • Centralized inside the ResourceNotificationService? ApplicationExecutor, AzureProvisioner, and anyone else publishing updates would automatically parentId property get it set based on if the resource implements IResourceWithParent.
  • Added to AzureProvisioner?

@davidfowl
Copy link
Member

I would add it to the AzureProvisioner for now.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 12, 2024

Done:

image

@JamesNK JamesNK merged commit 9abbb30 into main Nov 12, 2024
9 checks passed
@JamesNK JamesNK deleted the jamesnk/resource-parent branch November 12, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants