-
Notifications
You must be signed in to change notification settings - Fork 712
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
Remove MappedNode & generalise render logic #233
Conversation
f052ddd
to
2ce7065
Compare
2ce7065
to
23bdae3
Compare
Could you write a few sentences for context? At a minimum, the problem that motivates this PR, and a high-level description of the implementation? |
23bdae3
to
0899c89
Compare
This is a refactoring on the path to synthesising the containers topology from endpoints topology via the process topology. To do this we need the renderers to be able to recurse - effectively what I'm aiming for is:
This isn't all the way there yet, just a step in that direction (getting the Renderer interface right I think). I removed MappedNode as its redundant, with the introduction of DetailedNode.Merge. It has the nice side effect of propagating Origin nodes correctly for nodes without any adjacency info. |
// Renderer is something that can render a report to a set of RenderableNodes | ||
type Renderer interface { | ||
Render(report.Report) report.RenderableNodes | ||
EdgeMetadata(rpt report.Report, localID, remoteID string) report.AggregateMetadata |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This feels really refreshing. Whenever we can eliminate a needless intermediate form, wonderful. Thanks! Those comments addressed, LGTM. |
0899c89
to
1e92e7d
Compare
Remove MappedNode & generalise render logic
Towards #228