-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Extend protobuf messages #21
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
sergeymatov
commented
May 20, 2025
- Make interfaces IPs repeatable.
- Allow BGP Router to have list of networks to advertise explicitly.
- Bump version to match git tags.
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.
Please split actual changes and formatting clean-ups into separate commits. This will really help for review.
69b4ed6
to
0c2d9c4
Compare
0c2d9c4
to
bc03087
Compare
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.
Hi Sergey,
I provided some feedback on my first pass, please engage in the discussion! 🙂 You marked the PR as ready for a second review, but:
-
You didn't address my comment from the first pass. Please do separate the formatting changes from the actual changes. I know it takes time, but reviewing takes time, too, and at the end of the day we all benefit from clean and logical commits in the repo.
-
I had a question on the PR description (regarding the tags you mentioned), you did not reply 😢
-
The commit title does not say what the commit does, and it's not even consistent with the PR title. I'm not sure what I'm looking at when looking at the changes. And please don't leave the commit description empty, unless for super trivial changes! You have a PR description, why not keep it in the commit if it's relevant? And providing one line to explain the motivation for the change is super quick but always helpful.
-
You need to sign off your commit, please.
This commit adjusts GRPC model with following changes: - ipaddrs filed of Interface message is now repeated and plural. - BgpNeighbor message has new repeated field networks used to carry IP prefixes to be advertused explicitly Patch version is increased to 0.2.2 Signed-off-by: Sergey Matov <sergey.matov@githedgehog.com>
Signed-off-by: Sergey Matov <sergey.matov@githedgehog.com>
bc03087
to
ee3d819
Compare
@qmonnet thanks for the review, please find responses
|
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.
So much easier to understand now, thank you! 🥰
Looks good to me