-
Notifications
You must be signed in to change notification settings - Fork 148
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
add reportMatrix module #1043
add reportMatrix module #1043
Conversation
Hi @NickBouwhuis, Thanks for this contribution and for adding yourself to the friends file! I'm going to add a few comments of review. |
As you can see, all minor things :) |
Where did you add these comments? I'm guessing I am missing something 😅 |
I think there is a final step after adding the comments that says "require changes" that I forgot. Are you able to see them now? |
Thanks a lot for the contribution. I will merge this into the new release. |
I just realized that probably the transaction Id generated in the constructor is not good enough. According to the documentation, the transaction id is used to avoid duplicated calls. Hence, it should be generated before every call. Am I reading this wrong? Anyway, I will patch it. |
Patched. Please, use this https://github.com/nttgin/BGPalerter/releases/tag/v1.32.0 in your installation. |
Yes. That is correct. My bad. I only tested it with |
Np, patched. Please, run the latest binary for some days and let me know if you experience any issue. |
Added the reportMatrix module and documentation for it.
Matrix requires messages to include a
txId
(as can be read in the Matrix documentation here) and thus the reportHTTP module cannot be used.