Skip to content

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

Merged
merged 2 commits into from
May 27, 2025
Merged

Conversation

sergeymatov
Copy link
Contributor

  • Make interfaces IPs repeatable.
  • Allow BGP Router to have list of networks to advertise explicitly.
  • Bump version to match git tags.

Copy link
Member

@qmonnet qmonnet left a 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.

@sergeymatov sergeymatov force-pushed the pr/smatov/repeated-ips-interface branch 2 times, most recently from 69b4ed6 to 0c2d9c4 Compare May 26, 2025 14:58
@sergeymatov sergeymatov requested a review from qmonnet May 26, 2025 14:58
@sergeymatov sergeymatov force-pushed the pr/smatov/repeated-ips-interface branch from 0c2d9c4 to bc03087 Compare May 26, 2025 17:17
Copy link
Member

@qmonnet qmonnet left a 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>
@sergeymatov sergeymatov force-pushed the pr/smatov/repeated-ips-interface branch from bc03087 to ee3d819 Compare May 27, 2025 11:09
@sergeymatov
Copy link
Contributor Author

@qmonnet thanks for the review, please find responses

  • It's makes sense, since linter was not invoked previously, so it affects not only introduced changes. Separated into different commits
  • We have some day match the tag of Golang component, not only Rust crate. I don't see any changes have them separate, but it's another topic to discuss
  • Adjusted commit message
  • it's a classic one :)

@sergeymatov sergeymatov requested a review from qmonnet May 27, 2025 11:13
Copy link
Member

@qmonnet qmonnet left a 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

@sergeymatov sergeymatov merged commit aa8a6e5 into master May 27, 2025
4 checks passed
@Frostman Frostman deleted the pr/smatov/repeated-ips-interface branch June 2, 2025 16:59
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