search

eBay / go-ovn

A Go library for OVN Northbound/Southbound DB access using native OVSDB protocol
Apache License 2.0
108 stars 59 forks source link

Fix missing parameters of execution function of OvnCommand #79

Closed fyuan1316 closed 4 years ago

fyuan1316 commented 4 years ago

add the missing parameters, so we can execute ovnCommand directly.

Before:

ocmd, _ := ovndbapi.LSAdd("ls1") ovndbapi.Execute(ocmd)

After:

ocmd, _ := ovndbapi.LSAdd("ls1") ocmd.Execute()

fyuan1316 commented 4 years ago

@hzhou8 @noah8713 It looks like the parameters are forgotten, if this is the original design, please correct me ：）

fyuan1316 commented 4 years ago

add example and some description ：）

noah8713 commented 4 years ago

@fyuan1316 : Did CI pass for this change?

noah8713 commented 4 years ago

Thanks for the explaination!

hzhou8 commented 4 years ago

This PR should be squashed before merge.

noah8713 commented 4 years ago

This PR had two commits in same PR. Merge didn't complain for rebase so I merged it instead of squash.

hzhou8 commented 4 years ago

@noah8713 The second commit is a fix of the previous commit (a revision based on review comment), which is why they should be squashed. In another words, they belong to same change, and the principles is one commit for one change.

noah8713 commented 4 years ago

@hzhou8 : I just checked the commits. Since commit https://github.com/eBay/go-ovn/pull/79/commits/40b307c93fbdf702fba1698989bf46b3d814cb0d is the one on top of https://github.com/eBay/go-ovn/pull/79/commits/0f9821be95ba27b9273b028633a249d23cbe6b25 , review comments code got addressed fine. So ideally what you are proposing is user should be sending one PR for each commit instead of two commits in same PR. Do you want me to revert the merge? We can ask user to send one PR with both two commits squashed into one.