-
Notifications
You must be signed in to change notification settings - Fork 155
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
Plugin: Migrate to @grpc/grpc-js to resolve problems when IPv6 is disabled #135
Conversation
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Works with Ubuntu 16.04 with both Grafana 6.7.x and 7.0.x. Still need to figure out how to disable IPv6 and test again. |
Tried disable IPv6 using these instructions:
Can still run the v1.0.12 version of image renderer plugin. Not sure what else to do? |
Was able to turn off IPv6 completely using a boot option (instructions)
|
@marefr Yayyy |
Since we're upgrading zeit/pkg need to verify these changes works on Windows and Darwin as well - had issues with that earlier. |
Verified works on Windows. Who can help out testing this on Darwin? |
@marefr I can do that. |
I tested it on OS X @marefr, works. |
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.
LGTM
Added a minor change that would be nice before merging
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Thanks @aknuds1! Great work. |
Migrate to @grpc/grpc-js, in order to solve #48, since the IPv6 issue is presumably not present in the pure-JS version of gRPC.
I had to make a lot of changes because of TypeScript compilation errors that appeared, I can only guess that grpc-js has stricter type definitions. In particular, I had to move gRPC methods out of classes, because the class types didn't have the right interface.
In retrospect, I think I might understand how to give the class types the right interface (basically make them indexable by string), so I could probably re-introduce classes for containing the methods.
Fixes #48
NB:
How should I best test this? I tested it manually, by rendering a panel in Grafana and it does work.