Support for connect native services in topology view.#12098
Support for connect native services in topology view.#12098kisunji merged 2 commits intohashicorp:mainfrom
Conversation
|
Hey @apollo13 Thanks for raising this PR! Just letting you know that we didn't skip over this, our front-end eng will be looking over this and giving some guidance on the tests either this week or the next 👍 |
|
Hey @apollo13 👋 Quick note to let you know I'll be picking this up, I'll need a little time to look over the linked issues and get a handle on things so I'll probably be back in a couple o' days or so with questions/queries, just wanted to let you know it's on my radar ty! |
|
Hey @apollo13 👋 Apologies for the delay in coming back to you here. As most of the changes here are golang related to I've asked somebody on the team to swing by and give this a 👁️ . From the JS side, as there are very minor changes I'd be happy for this to be merged as is (if the go bits are fine) and we can take it from there, so I'll leave approval up to the go folks. |
agent/consul/state/catalog.go
Outdated
| } | ||
| if _, ok := tproxyMap[sn]; !ok && downstreamSources[sn.String()] != structs.TopologySourceRegistration { | ||
| // If downstream is not a transparent proxy, remove references | ||
| if _, ok := tproxyMap[sn]; (!ok && !downstream.Service.Connect.Native) && downstreamSources[sn.String()] != structs.TopologySourceRegistration { |
There was a problem hiding this comment.
Is there a reason for () around (!ok && !downstream.Service.Connect.Native)?
There was a problem hiding this comment.
No, probably a left-over -- will fix.
kisunji
left a comment
kisunji
left a comment
There was a problem hiding this comment.
LGTM!
Would you be able to update a test at the handler level here:
https://github.com/hashicorp/consul/blob/main/agent/ui_endpoint_test.go#L998
I think all it needs is an assertion for a mock NodeService with svc.Connect.Native: true
|
@kisunji I think I got a test running; I added a new node for it because otherwise those would be a mess… |
|
What I do not know yet: Should I include the static assets ( |
Ah you can skip that; we have CI to automatically generate them |
kisunji
left a comment
kisunji
left a comment
There was a problem hiding this comment.
Looks like the test ended up being more of a hassle than I expected, but thank you for being so thorough!
|
Waiting on the team to give a final 👍 before merging |
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/586713. |
|
Thanks a lot, this will make topology view much nicer when used with traefik :)
…On Wed, Feb 16, 2022, at 22:52, Chris S. Kim wrote:
Merged #12098 <#12098> into main.
—
Reply to this email directly, view it on GitHub
<#12098 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAT5C5YBEGPRGDYP74HQN3U3QMANANCNFSM5L7YLL2Q>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
5 participants
This is a draft implementation for #12096. Misses tests and probably cleanups of the JS code :)