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.

Sign up for GitHub

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

Jump to bottom

gobgpd: init at 2.26.0 #111316

Merged
merged 3 commits into from Apr 9, 2021
Merged

gobgpd: init at 2.26.0 #111316

merged 3 commits into from Apr 9, 2021
+173 −0

Conversation

higebu
Copy link
Contributor

@higebu higebu commented Jan 30, 2021 •
edited

Motivation for this change

GoBGP is an open source BGP implementation. https://github.com/osrg/gobgp
Related to: #111083

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after) 
    $ nix path-info -S /nix/store/by6m9mkjfphj7q6cq5vx58jwj904dp8a-gobgpd-2.26.0
/nix/store/by6m9mkjfphj7q6cq5vx58jwj904dp8a-gobgpd-2.26.0          15990496
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This change is Reviewable

@ofborg ofborg bot requested a review from kalbasit January 30, 2021 15:26
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 111316 run on x86_64-linux 1

1 package blacklisted:
  • tests.nixos-functions.nixos-test
1 package built:
  • gobgpd

@higebu higebu changed the title gobgpd: init at 2.23.0 gobgpd: init at 2.24.0 Mar 25, 2021
@higebu
Copy link
Contributor Author

higebu commented Mar 25, 2021

Result of nixpkgs-review pr 111316 1

1 package built:
  • gobgpd

@higebu
Copy link
Contributor Author

higebu commented Mar 25, 2021

@SuperSandro2000 Can I do anything to merge this pr?

@higebu higebu changed the title gobgpd: init at 2.24.0 gobgpd: init at 2.25.0 Mar 25, 2021
SuperSandro2000
SuperSandro2000 approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package LGTM

not sure about the module or the test.

@higebu
Copy link
Contributor Author

higebu commented Mar 26, 2021

@SuperSandro2000 Thank you for your approval.

@higebu higebu changed the title gobgpd: init at 2.25.0 gobgpd: init at 2.26.0 Apr 5, 2021
@higebu
Copy link
Contributor Author

higebu commented Apr 5, 2021

@kalbasit Cloud you review this PR?

aanderse
aanderse requested changes Apr 5, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module looks good, just a few minor suggestions.
Assuming I knew anything about this software (which I do not) I would probably say the test looks fine too.

👍

nixos/modules/services/networking/gobgpd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/gobgpd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/gobgpd.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/gobgpd.nix Outdated Show resolved Hide resolved
@higebu
Copy link
Contributor Author

higebu commented Apr 6, 2021

@aanderse I fixed this module based on your review comments. Thank you.

aanderse
aanderse approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module portion of this PR looks good to me. Great work! 👍

@higebu
Copy link
Contributor Author

higebu commented Apr 6, 2021

@aanderse Thank you!

mweinelt
mweinelt reviewed Apr 7, 2021
View reviewed changes
nixos/tests/gobgpd.nix Outdated
};

nodes = {
node = { ... }: {
Copy link
Member

@mweinelt mweinelt Apr 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should likely have multiple nodes to establish bgp sessions amongst each other, and check that this works.

mweinelt
mweinelt reviewed Apr 7, 2021
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some open questions.

@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

Sorry to mention this so late, but you need to split the changes into multiple commits.

  • Package: gobpgd: init at 2.26.0
  • Module: nixos/gobpgd: init
  • Test: nixos/tests/gobgpd: init

@higebu
Copy link
Contributor Author

higebu commented Apr 9, 2021

@mweinelt Thank you for your comment. I fixed commits.

@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

Looks awesome! Let's wait for the tests and then go for the merge! 🎆

@GrahamcOfBorg test gobpgd

@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

@GrahamcOfBorg build nixosTests.gobgpd

@mweinelt mweinelt merged commit f882b05 into NixOS:master Apr 9, 2021
@higebu
Copy link
Contributor Author

higebu commented Apr 9, 2021

@mweinelt Thank you!

@mweinelt
Copy link
Member

mweinelt commented Apr 9, 2021

No, thank you! 🕺

@higebu higebu mentioned this pull request Jun 7, 2021
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse approved these changes

@mweinelt mweinelt mweinelt approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@higebu @SuperSandro2000 @mweinelt @aanderse