Conversation
| // | ||
| // pb, err := bgp.NewPathBuilder("10.244.1.0/24", "192.168.1.1") | ||
| // pb, err := bgp.NewPathBuilder("2001:db8::/64", "2001:db8::1") | ||
| func NewPathBuilder(cidr string, nextHop string) (*PathBuilder, error) { |
There was a problem hiding this comment.
Is the plan to extend this with more logic in the future? I like the builder pattern, but almost all of the logic for this is already inside this constructor. I wonder if it would just be simpler to not have the builder pattern and just have a function that takes in the isWithdrawal bool and returns the apiutil.Path:
func BuildPath(cidr, nextHop string, isWithdrawal bool) (*apiutil.Path, error) There was a problem hiding this comment.
That's a fair point. Even the AI wanted me to consolidate down to using a bool in the signature. Mostly, I just really don't like boolean parameter values, because people usually just end up passing true / false in on them which doesn't lend itself towards description. This can be solved by making a variable like withdrawPath := true and then passing BuildPath(myCIDR, nextHop, withdrawPath) but in practice this almost never happens.
Additionally, maybe this is just sky high hopes, but I was thinking that Path's are pretty integral to what we use BGP for so having a more robust builder that people are already familiar with might encourage extensibility? That being said, we haven't done THAT much with Path over the years.
All that to say, I could go either way.
There was a problem hiding this comment.
I don't have a strong preference here and well... I do like the builder pattern because it's quite clean 😬. Fine with me to leave it in!
catherinetcai
left a comment
catherinetcai
left a comment
There was a problem hiding this comment.
Oops, I realized I didn't have blocking changes here and should have hit approve.
aauren
force-pushed
the
bump_versions_pre_2.7_release
branch
from
19870b0 to
ab19b3d
Compare
aauren
force-pushed
the
bump_versions_pre_2.7_release
branch
from
ab19b3d to
689edb5
Compare
3 participants
FYI @mrueg @catherinetcai - This should prep us for a v2.7.0 release of kube-router. There have been a lot of changes lately, so I think that we're well ready.
Unfortunately, the update from gobgp v3.37.0 -> v4.2.0 was a lot more work than I thought it would be when I originally started working on this release earlier today.