-
Notifications
You must be signed in to change notification settings - Fork 682
Identify replica sets based on owner info provided by DCP, group applications by their replica set in resource dropdowns, add tests for console logs application resource grouping #4908
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
Conversation
@JamesNK Could you advise on what to do about |
private static ImmutableArray<OwnerReferenceSnapshot> GetOwners(CustomResource resource) | ||
{ | ||
return [ | ||
..resource.Metadata.OwnerReferences | ||
.Select(ownerReference => new OwnerReferenceSnapshot(ownerReference.Kind, ownerReference.Name, ownerReference.Uid)) | ||
]; | ||
} |
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.
It's odd for something to have multiple owners. What are scenarios that would cause multiple owners?
cc @karolz-ms
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 is a K8s API concept and probably not the best name: it is a list of objects that depend on the given object. https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/object-meta/#System Most common scenario is when there is some sort of N:M relationship that is modeled as a separate object, the motivation for multiple owners being keeping the model consistent and cascade deletion.
For example (we are not doing this today, but we might in future): an Endpoint is exposed on a Network, implements a Service, and is implemented by an instance of a Container or Executable. The Network, Service, and implementing Container/Executable could all be "owner" objects.
src/Aspire.Dashboard/Components/Controls/ResourceSelectOptionTemplate.razor
Outdated
Show resolved
Hide resolved
src/Aspire.Dashboard/Components/Controls/ResourceSelectOptionTemplate.razor
Outdated
Show resolved
Hide resolved
# Conflicts: # src/Aspire.Dashboard/wwwroot/css/app.css
# Conflicts: # src/Aspire.Dashboard/Resources/ControlsStrings.resx
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.
I'm fine with the changes in this PR except everything to do with getting and returning Owner info to the dashboard. It doesn't fix anything, it makes things more complicated, and it adds fields to the proto contract that we should think careful about before changing.
Think Owner changes should be removed from this PR.
# Conflicts: # src/Aspire.Hosting/PublicAPI.Unshipped.txt
Currently, we are grouping applications based on whether their display names are identical, instead of using the
ExecutableReplicaSet
property from DCP.The approach I took was
(replica set)
with(application)
on traces/logs resource dropdowns, because the OTLP applications there can be grouped, but are not replica sets.Fixes Unit Tests for Resource Name display text #1411
Fixes Different instances of same resource type grouped as replicas in dashboard #4103
Fixes Identify replica sets for dashboard #1233
Microsoft Reviewers: Open in CodeFlow