Skip to content
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

Merged
merged 6 commits into from
Feb 23, 2023
Merged

add reportMatrix module #1043

merged 6 commits into from
Feb 23, 2023

Conversation

NickBouwhuis
Copy link
Contributor

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.

@massimocandela massimocandela changed the base branch from dev to node18 February 22, 2023 14:31
@massimocandela
Copy link
Member

Hi @NickBouwhuis,

Thanks for this contribution and for adding yourself to the friends file! I'm going to add a few comments of review.

@massimocandela
Copy link
Member

As you can see, all minor things :)
I can merge this for the next release (which will go out soon)

@NickBouwhuis
Copy link
Contributor Author

As you can see, all minor things :) I can merge this for the next release (which will go out soon)

Where did you add these comments? I'm guessing I am missing something 😅

config.yml.example Outdated Show resolved Hide resolved
docs/reports.md Show resolved Hide resolved
src/reports/reportMatrix.js Outdated Show resolved Hide resolved
@massimocandela
Copy link
Member

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?

@massimocandela
Copy link
Member

Thanks a lot for the contribution. I will merge this into the new release.

@massimocandela
Copy link
Member

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.

@massimocandela
Copy link
Member

Patched. Please, use this https://github.com/nttgin/BGPalerter/releases/tag/v1.32.0 in your installation.

@NickBouwhuis
Copy link
Contributor Author

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.

Yes. That is correct. My bad. I only tested it with -it. The transaction ID needs to be unique with every message.

@massimocandela
Copy link
Member

Np, patched. Please, run the latest binary for some days and let me know if you experience any issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants