-
Notifications
You must be signed in to change notification settings - Fork 58
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
chore(wakucanary): add canary tool #1205
Conversation
Jenkins BuildsClick to see older builds (13)
|
ccf5df6
to
c2716ac
Compare
c2716ac
to
fe1d0c4
Compare
This first approach ensures:
Example: make wakucanary Peer is reachable on port $ ./build/wakucanary --storenode=/ip4/8.210.222.231/tcp/30303/p2p/16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD
$ echo $?
> 0 Peer is not reachable on port $ ./build/wakucanary --staticnode=/ip4/8.210.222.231/tcp/1000/p2p/16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD
$ echo $?
> 1 Questions:
|
Thanks for this and good work! I'll do a more in depth review tomorrow, but a couple of initial comments:
|
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.
5bf48e0
to
445c7b3
Compare
Thanks for this PR. I built it locally and tried using it against one of the hosts in our
Some questions:
|
Perhaps you can try with |
Ah, fair point, but yeah, would be good, especially since we want to use the multiaddress from consul to generate checks, and the one in consul are the ones with DNS entries. Or more specifically they are the ones the node API gives me:
And that's why I'm using that. So either add support for DNS in the canary, or add support for returning the multiaddress with the IP in the API. Or I have to do some kind of disgusting hacky regex replacement to make the DNS one into an IP one. But yes, that does indeed work:
Nice. |
Oh, and would it be a lot of work to add 1 letter alias flags? |
Very nice:
Not sure about that EDIT: Waaait, I checked for |
Okay, now it detects lightpush as expected:
Nice. And But yeah, considering you give a DNS multiaddress via RPC API, and that's the address we expose via https://fleets.status.im/ it would make sense to support the addresses we publish. |
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. Useful tool! Congrats on your first PR for nwaku. :) I agree that supporting DNS address resolution will be useful as that is how we publish most addresses. This can be a subsequent PR, though, so feel free to merge this first.
I'm fine with DNS support being added in separate PR, as this is an already working change that can be merged. |
fe3219e
to
a7a354c
Compare
Great! Thanks for the comments. Will address the DNS support in another PR. Just rebased and fixed a typo, no changes. Can I get ✅ again? Edit: Wops, looks like its not needed to aprove again. |
closes #754