[ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries.

[Joe Stringer]

On 21 July 2017 at 07:52, Darrell Ball <dball at vmware.com> wrote:
>
>
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Date: Friday, July 21, 2017 at 5:24 AM
> To: Darrell Ball <dball at vmware.com>, Joe Stringer <joe at ovn.org>
> Cc: ovs dev <dev at openvswitch.org>, Ben Pfaff <blp at ovn.org>
> Subject: Re: [ovs-dev] [patch_v1 1/2] System Tests: Allow SNAT address variability retries.
>
>     On 21.07.2017 05:40, Darrell Ball wrote:
>     > The discussion about the ‘Area’ prefix has come up again, even after Ben had commented about it
>     > and after I had pointed folks to the submitting-patches.rst, which allows flexibility in choosing an
>     > ‘Area’ prefix by the patch submitter.

Hi Darrell,

It seems like we got off on the wrong foot on this one. You had asked
me to look at the patches, so I fetched them, looked at the somewhat
trivial feedback, applied that locally and tested the patches. Given
that it seemed like I had done all the work locally that you would
have done if you respin a v2, I figured that I would propose to just
push them as-is from my local tree. To be explicit about the changes I
had made, I responded on the list to highlight the changes. This gave
you a chance to review what I had done, which you have done. Clearly
this topic is important enough to you that you preferred I don't apply
the patches as I had prepared them - so I backed off to allow you to
send a v2.

>     >
>     > In this thread, it was again suggested that the use of ‘Area’ prefix ‘System Tests’ needs to change to ‘System-traffic’
>     > Below, I list some the history regarding previous commits of system-traffic.at as a single patch.
>     > Different people have had different preferences and those seem to have been tolerated in the past.
>     >
>     > Hence, I would like know what has changed recently such that the documented
>     > (submitting-patches.rst) and historical flexibility (see previous patches) regarding the
>     > ‘Area’ prefix is no longer tolerated ?
>     >
>     > My suggestion is that we don’t continue along these new lines and rather stay flexible, as the
>     > work will be more productive in such an environment.
>     >
>     > Darrell
>
>     Hi Darrell.
>
>     Looks like I should say something because I raised this issue.
>     First of all I want to say that it's my own opinion and you're
>     completely free to disagree with it, but I need to clarify my
>     position.
>
>     About system-traffic.at related patches:
>
>     You mentioned below commits which has 'tests' and 'system-tests'
>     prefixes. 'tests' is fine, because it is the folder name. Maybe
>     authors should be more specific with patches where only one file
>     changed, but it doesn't really matter.
>     From the other side 'system-tests' is not the name of file or
>     folder, it's the area. But if you'll go back to the history,
>     all of these patches was committed when where was only one file
>     responsible for system tests (one patch is an exception introduced
>     after appearing of system-ovn.at). So it was, actually, fine
>     at the time of submission.
>
> Really, so that would mean as long as only one file exists in an area, that the
> associated naming used in ‘Area’ prefixes is flexible, per your rules, right ?
> You seem to be on my side of the discussion now.
>
>
>     Today we have at least 4 types of system tests and it'll be nice
>     to have more detailed information directly in subject instead of
>     looking to the patch itself.
>
> You just a few lines above stated that directory name (eg Tests) for ‘Area’ prefixes is ok.
> Are not directory names even a wider scope than a few files and even more ambiguous.
> Directories have many files covering many different parts of the code, like ‘tests’, ‘datapath’ and ‘windows-datapath’.
> So, you are saying “One file is ok, many different files are ok, but 4 similar files are bad” ?, I see.

I believe that the reason this is flexible is that sometimes patches
change files across multiple systems. That's actually the trickiest
thing to pick a name for, and occasionally patches end up omitting the
area entirely for this reason. However when it comes to a patch
changing a single file, typically the simplest way to pick a name is
to take the filename.

>     Additionally, this mailing list is actually not the primary place
>     for reviewing patches for kernel datapath. They are here only for
>     information and backporting. 'datapath-windows' prefix needed to
>     filter patches targeted for windows because there are only few
>     persons who works on that and able to review and test.
>     So, the main areas for patches in this mailing list are general
>     management code, userspace actions and userspace datapath.
>     Userspace datapath contains too many files/modules to not
>     mention them in subject line. So, if you're submitting patch
>     with 'conntrack' prefix, everybody knows that it's all about
>     connection tracking in userspace.
>
>     Beside all of that: isn't it a good habit to use most commonly
>     used prefixes like 'system-traffic' or 'conntrack' instead of
>     making the new one?
>     If everybody will use their own preferable prefixes, git history
>     will become a total mess. And that is the main concern.
>
> That would mean the existing history we have, for the examples stated already,
> are a problem and you seem to have no problem tracing the git
> history even when ‘tests’ and ‘System-tests’ are used in place of ‘System traffic’, as you
> mentioned above. So where is the problem ?

I can see cases where I've been interested in listing all patches
relevant to an area, where filtering to specific files is not an
appropriate solution. In this case, the fewer possible different areas
I need to consider, the easier it is to script against the git logs.
For this kind of use case, improving consistency is the most useful
thing. If one label was used consistently at some point in history,
then you can consider everything up until that point trivially. If it
changes at some point in the history, then obviously I have to go and
update my filter to include the new one as well. We can't do much
about that. However, for incoming patches going forward, ideally we
don't move towards a situation with several different ways to refer to
changes to particular areas as this will complicate this kind of
usage. This is, for instance, why I prefer that patches consistently
use all lower case (unless the filename has capitals); consistently
use dashes rather than spaces and so on.

Cheers,
Joe