-
Notifications
You must be signed in to change notification settings - Fork 881
fix: track JetBrains connections #10968
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
Based on tcp forwarding instead of ssh connections
7948ab5
to
e7f9516
Compare
@code-asher Try |
Verified it works! 🎉 |
fe83fbd
to
9dede7f
Compare
Ah wait moving back to draft, there seems to be an issue... |
9dede7f
to
9840a69
Compare
The two issues I ran into, just FYI:
I think it is all sorted now, doing some more testing to be sure. |
OK, verified it all works as expected in a deployment. Test is failing in CI though. Passes for me locally... |
Ooof, this test is quite strange too. That sounds annoying to debug... Use |
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
64ea05d
to
6e8f235
Compare
Will this also work for Fleet? |
e79b01c
to
ec59e7e
Compare
I am not sure, we will need to test. |
ec59e7e
to
90e6e88
Compare
9cf5082
to
7fd04cb
Compare
The test name only shows up in the process name if you are running that test directly so we have to spawn a separate process instead.
7fd04cb
to
a75ed6c
Compare
OK, sorted the test. The test name only shows up in the process name if you run that specific test directly, so when we are running the entire suite the name does not show up. What I did was spawn an external process instead. Not sure if I did it in an idiomatic way or if there even is an idiomatic way, with Node I would just We could also try spawning something like |
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.
Looks good, let's get this in and test it a bit.
Fixes #9673
Possibly we will need to find a better string to match on, but this one seems OK to me.
Unit test passes but have not tested an actual deploy yet.