On 10/3/23 14:10, Viacheslav Galaktionov via dev wrote: > On 9/29/23 17:02, Simon Horman wrote: >> On Mon, Sep 04, 2023 at 07:45:07PM +0400, Viacheslav Galaktionov via dev >> wrote: >>> Currently, if the user wants to track related connections, they have to >>> specify a helper in all CT actions, which contradicts the behaviour >>> described in the documentation. >>> >>> Fix this by using the helper committed along with the connection whenever >>> a given CT action does not specify a helper of its own. >>> >>> Signed-off-by: Viacheslav Galaktionov >>> <[email protected]> >>> Acked-by: Ivan Malov <[email protected]> >> Hi Viacheslav, >> >> thanks for your patch-set. I agree this is a problem that ought to be >> fixed. > Hi Simon! > > Thanks for the feedback, much appreciated! >> >> Some feedback on the patchset from my side: >> >> 1. The test added by patch 1/2 fails without patch 2/2. >> I think it would be good to reverse the order of the patches in the >> series. > Thanks, I'll reverse the order of patches on my next submit. My original > logic was to "introduce" the problem by making it visible and then "fix" > it, but I suppose having tests fail is not ideal after all.
There should be no point in git history where tests fail, ideally. Otherwise it's hard to bisect. I'd suggest to just squash both patches into one. This makes backporting of fixes easier, if needed, and clearly shows which fix the test in for while looking through the git history. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
