[Engine-devel] Proposal for new commit msg design for engine commits

Alon Bar-Lev alonbl at redhat.com
Sat Jul 20 18:53:06 UTC 2013 


----- Original Message -----
> From: "Eyal Edri" <eedri at redhat.com>
> To: "Alon Bar-Lev" <alonbl at redhat.com>
> Cc: "infra" <infra at ovirt.org>, "engine-devel" <engine-devel at ovirt.org>, "Fabian Deutsch" <fabiand at redhat.com>
> Sent: Saturday, July 20, 2013 9:41:56 PM
> Subject: Re: [Engine-devel] Proposal for new commit msg design for engine commits
> 
> This change to commit template has nothing to do with CI.
> it's a change that should reflect updated components relevance to the commit
> code.

This commit template is mostly invalid, as touching more than one 'subsystem' is possible, and has not enough granularity.

For example, database change should trigger what?
Infra change should trigger what?
A change of both user interface and command should trigger what?
So you end up with:

userportal: storage: core: db: some message

Just to make who happy?

And maybe there are 50 tests of network, and you need only 5 of them for the specific change, how do you mark it, so now a developer need to know any test? what if you add one tomorrow which is relevant to a similar change? how do you inform the developer that now he needs 6?

Why should it be the developer responsibility and not the quality ensuring engineer responsibility to determine which tests should run and when?

As far as this template was not actually used for anything but humans, it was not that important, but once you formalize it as an interface, I step forward and state that the subject line is not the right tool for the task at hand (or any for this matter).

The fact that you have in each commit are the sources that are modified, all the other data is just plain noise. From the sources that are modified you should be able to derive a test plan with high chance that this test program will be correct. Human intervention should be possible by ordering special tests that are outside of the standard policy, for cases in which the standard policy of deriving tests from sources is too narrow.

Regards,
Alon


> 
> Nevertheless, i have no problems with your suggestions for metadata per
> directory to map all ovirt code.
> any suggestion how to push it forward?
> 
> Eyal.
> 
> ----- Original Message -----
> > From: "Alon Bar-Lev" <alonbl at redhat.com>
> > To: "Eyal Edri" <eedri at redhat.com>
> > Cc: "infra" <infra at ovirt.org>, "engine-devel" <engine-devel at ovirt.org>,
> > "Fabian Deutsch" <fabiand at redhat.com>
> > Sent: Saturday, July 20, 2013 9:34:13 PM
> > Subject: Re: [Engine-devel] Proposal for new commit msg design for engine
> > commits
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "Eyal Edri" <eedri at redhat.com>
> > > To: "infra" <infra at ovirt.org>
> > > Cc: "engine-devel" <engine-devel at ovirt.org>, "Fabian Deutsch"
> > > <fabiand at redhat.com>
> > > Sent: Saturday, July 20, 2013 8:34:28 PM
> > > Subject: Re: [Engine-devel] Proposal for new commit msg design for engine
> > > commits
> > > 
> > > OK, Rome wasn't built in a day.
> > > 
> > > To move things forward,
> > > I propose we'll just improve current commit header template to include
> > > more
> > > relevant code
> > > areas [1], and start looking into mapping all code to the relevant
> > > components
> > > (either via renaming folders, adding a metadata file under each folder
> > > mapping the files/classnames/directory names or using automated tools
> > > like
> > > sonar)
> > 
> > Again, and I am sorry, but I disagree of any relationship between commit
> > message and CI.
> > 
> > It will be simple to add metadata to sources, and have CI run tests based
> > on
> > actual source change thus probable impact, this way we won't be exposed to
> > human errors, nor make commit message unusable for actual history.
> > 
> > All we need is someone to take ownership of the task of adding metadata to
> > source tree.
> > 
> > As I proposed this can be either within every source using special
> > signature,
> > or can be in a directory at special file, for example .ovirt-metadata, and
> > have the mapping between source component to relevant tests at a simple
> > text
> > file at source root.
> > 
> > Regards,
> > Alon Bar-Lev.
> > 
> > > 
> > > [1] instead of               <core | restapi | tools | history | engine |
> > > userportal | webadmin>
> > >     change to something like <core | restapi | tools | userportal |
> > >     webadmin
> > >     | network | storage | virt | packaging>
> > > 
> > > Eyal.
> > > 
> > > ----- Original Message -----
> > > > From: "Moran Goldboim" <mgoldboi at redhat.com>
> > > > To: "Eyal Edri" <eedri at redhat.com>
> > > > Cc: "Fabian Deutsch" <fabiand at redhat.com>, "engine-devel"
> > > > <engine-devel at ovirt.org>, "infra" <infra at ovirt.org>
> > > > Sent: Sunday, July 14, 2013 6:07:05 PM
> > > > Subject: Re: [Engine-devel] Proposal for new commit msg design for
> > > > engine
> > > > commits
> > > > 
> > > > On 07/11/2013 11:57 AM, Eyal Edri wrote:
> > > > >
> > > > > ----- Original Message -----
> > > > >> From: "Fabian Deutsch" <fabiand at redhat.com>
> > > > >> To: "Eyal Edri" <eedri at redhat.com>
> > > > >> Cc: "Alon Bar-Lev" <alonbl at redhat.com>, "engine-devel"
> > > > >> <engine-devel at ovirt.org>, "infra" <infra at ovirt.org>
> > > > >> Sent: Thursday, July 11, 2013 11:41:24 AM
> > > > >> Subject: Re: [Engine-devel] Proposal for new commit msg design for
> > > > >> engine
> > > > >> commits
> > > > >>
> > > > >> Am Mittwoch, den 10.07.2013, 15:27 -0400 schrieb Eyal Edri:
> > > > >>> ----- Original Message -----
> > > > >>>> From: "Fabian Deutsch" <fabiand at redhat.com>
> > > > >>>> To: "Alon Bar-Lev" <alonbl at redhat.com>
> > > > >>>> Cc: "engine-devel" <engine-devel at ovirt.org>, "infra"
> > > > >>>> <infra at ovirt.org>
> > > > >>>> Sent: Tuesday, July 9, 2013 3:54:06 PM
> > > > >>>> Subject: Re: [Engine-devel] Proposal for new commit msg design for
> > > > >>>> engine
> > > > >>>> commits
> > > > >>>>
> > > > >>>> Am Dienstag, den 09.07.2013, 08:49 -0400 schrieb Alon Bar-Lev:
> > > > >>>>>
> > > > >>>>> ----- Original Message -----
> > > > >>>>>> From: "Yair Zaslavsky" <yzaslavs at redhat.com>
> > > > >>>>>> To: "Alon Bar-Lev" <alonbl at redhat.com>
> > > > >>>>>> Cc: "Eyal Edri" <eedri at redhat.com>, "engine-devel"
> > > > >>>>> <engine-devel at ovirt.org>, "infra" <infra at ovirt.org>
> > > > >>>>>> Sent: Tuesday, July 9, 2013 3:42:24 PM
> > > > >>>>>> Subject: Re: [Engine-devel] Proposal for new commit msg design
> > > > >>>>>> for
> > > > >>>>> engine     commits
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>> ----- Original Message -----
> > > > >>>>>>> From: "Alon Bar-Lev" <alonbl at redhat.com>
> > > > >>>>>>> To: "Eyal Edri" <eedri at redhat.com>
> > > > >>>>>>> Cc: "engine-devel" <engine-devel at ovirt.org>, "infra"
> > > > >>>>> <infra at ovirt.org>
> > > > >>>>>>> Sent: Tuesday, July 9, 2013 3:33:57 PM
> > > > >>>>>>> Subject: Re: [Engine-devel] Proposal for new commit msg design
> > > > >>>>>>> for
> > > > >>>>> engine
> > > > >>>>>>>      commits
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>>>
> > > > >>>>>>> ----- Original Message -----
> > > > >>>>>>>> From: "Eyal Edri" <eedri at redhat.com>
> > > > >>>>>>>> To: "engine-devel" <engine-devel at ovirt.org>
> > > > >>>>>>>> Cc: "infra" <infra at ovirt.org>
> > > > >>>>>>>> Sent: Tuesday, July 9, 2013 12:38:51 PM
> > > > >>>>>>>> Subject: Proposal for new commit msg design for engine commits
> > > > >>>>>>>>
> > > > >>>>>>>> Hi,
> > > > >>>>>>>>
> > > > >>>>>>>> You all probably know and familiar with 'ovirt-engine' git
> > > > >>>>>>>> hook
> > > > >>>>> for
> > > > >>>>>>>> commit
> > > > >>>>>>>> msg template [1].
> > > > >>>>>>>> this helps understand the general area of the patch in the
> > > > >>>>> project but it
> > > > >>>>>>>> lacks additional info that might
> > > > >>>>>>>> be valuable for scaling automatic tests in Jenkins CI.
> > > > >>>>>>>>
> > > > >>>>>>>> Let me explain:
> > > > >>>>>>>>
> > > > >>>>>>>> Infra team is working hard on expanding oVirt CI
> > > > >>>>>>>> infrastructure
> > > > >>>>> and
> > > > >>>>>>>> adding
> > > > >>>>>>>> more tests in jenkins (per commit/patch).
> > > > >>>>>>>> Adding important meta-data per patch can significatly improve
> > > > >>>>> the ability
> > > > >>>>>>>> to
> > > > >>>>>>>> run specific tests for each patch/commit,
> > > > >>>>>>>> and not waste valuable resources on Jenkins jobs that are not
> > > > >>>>> relevant to
> > > > >>>>>>>> the
> > > > >>>>>>>> code in the patch.
> > > > >>>>>>>>
> > > > >>>>>>>> So the idea is to add/expand current metadata per patch, in
> > > > >>>>>>>> the
> > > > >>>>> form of:
> > > > >>>>>>>> (either)
> > > > >>>>>>>>   1. expanding current header template to include more data
> > > > >>>>>>>>   like
> > > > >>>>> 'network'
> > > > >>>>>>>>   ,
> > > > >>>>>>>>   'setup', 'tools', 'virt'
> > > > >>>>>>> Please do not expand header, it is too short anyway.
> > > > >>>>>>>
> > > > >>>>>>>>   2. adding a new label with relevant tags for the patch,
> > > > >>>>>>>>   called
> > > > >>>>> e.g
> > > > >>>>>>>>   'METADATA: network, rest, virt'
> > > > >>>>>>> Having:
> > > > >>>>>>>
> > > > >>>>>>> CI-Tests: xxx
> > > > >>>>>>> CI-Tests: yyy
> > > > >>>>>>> CI-Tests: zzz
> > > > >>>>>>>
> > > > >>>>>>> Is much better.
> > > > >>>>>> I'm not sure we should have CI-Test - as we might use this for
> > > > >>>>> something else
> > > > >>>>>> besides CI.
> > > > >>>>>> Region_of_Interest as Dan suggests sounds better IMHO.
> > > > >>>>> I don't care how this is to be called.
> > > > >>>>> However, I do not think that commit message is the place for
> > > > >>>>> instructing CI to do anything.
> > > > >>>>> Commit message stays for good, it should contain information that
> > > > >>>>> is
> > > > >>>>> required a year from now.
> > > > >>>>> It has nothing to do with tests and such.
> > > > >>>> I agree with Alon here that the Ci informations don't belong in
> > > > >>>> the
> > > > >>>> commit msg.
> > > > >>>> My opinion is that a testcase should know what it covers. This
> > > > >>>> information from the testcase can then be used by any party to
> > > > >>>> determin
> > > > >>>> if the testcase should be run on a specific commit (which yields
> > > > >>>> informations about the changed paths, files, owner, author, etc
> > > > >>>> ...
> > > > >>>> which might be valuable).
> > > > >>> i think you're missing the point here.
> > > > >>> can you explain how do you propose a test case will know "what it
> > > > >>> covers"?
> > > > >>>
> > > > >>> let's take an example:
> > > > >>> let's say a new commit comes from ovirt-engine:
> > > > >>> http://gerrit.ovirt.org/#/c/16668/
> > > > >>> commit msg: "core: Use images instead of volumes at CDA message".
> > > > >>>
> > > > >>> now you have 1000 test cases (could be system or functional test).
> > > > >>> (let's assume that your infra can't support running 1000 tests per
> > > > >>> patch/commit).
> > > > >>>
> > > > >>> Some of these test suits checks network flow, some virt
> > > > >>> (migration/template
> > > > >>> for e.g), some host install, others storage flows and so on... ).
> > > > >>> you have one repo to clone (ovirt-engine, let's keep vdsm a side
> > > > >>> for
> > > > >>> a
> > > > >>> min), and to compile the project from for the tests.
> > > > >>>
> > > > >>> now given this scenario, please explain how will you know which
> > > > >>> test
> > > > >>> from
> > > > >>> the 1000 you have you'll run on it.
> > > > >>> do you believe that according to the author/path/filename you'll
> > > > >>> know
> > > > >>> if
> > > > >>> that patch involves storage or virt scenario?
> > > > >> Hey Eyal,
> > > > >>
> > > > >> Yes - I would at least give it a try to decide automagically what
> > > > >> tests
> > > > >> to run by looking at the change.
> > > > >> The main motivation for this is to not add another things which the
> > > > >> committer needs to take care of and IMO we humans tend to fail (at
> > > > >> some
> > > > >> point) on those boring tasks like adding correct metadata (let it be
> > > > >> a
> > > > >> typo or just not adding the correct testsuites/topis to be run).
> > > > > this process can be fully automatic via gerrit hooks & templates:
> > > > >
> > > > > typos or mistakes can be easily handles by gerrit hooks to help the
> > > > > committer fix the tags.
> > > > > as mentions previously, this logic can be done by the project
> > > > > maintainer
> > > > > and enforced by a template or hook.
> > > > >
> > > > > so for example - if someone writes a patch with patch header
> > > > > "webadmin:...."  ,
> > > > > then the tags he'll get to choose from are only relevant to ui/ux.
> > > > >
> > > > > so the only task a committer will have to do is to verify the default
> > > > > tags
> > > > > are relevant.
> > > > >
> > > > > i don't believe this is too much to ask for, considering the huge
> > > > > benefit
> > > > > that we'll get
> > > > > (stable code, less bugs, less ci breakage, mapping of specific code
> > > > > to
> > > > > relevant topic, statistics.. etc..)
> > > > >
> > > > >> But let's get back to your example.
> > > > >> Basically we can use the path and filename to determin what
> > > > >> testsuite
> > > > >> to
> > > > >> run.
> > > > >> Testsuites could be bound to paths and/or filenames and/or regexes
> > > > >> being
> > > > >> matched against the full filename.
> > > > >> Another approach would be to use a java package dependency tree to
> > > > >> determine which classes are directly and indirectly affected by a
> > > > >> change. This information can then be used to also build a set of
> > > > >> testsuites to be run. For example:
> > > > >> World uses Ocean uses Wale uses Cell - if Wale changes, we'll surely
> > > > >> want to run the testsuites assigned to the classes higher up in the
> > > > >> dependency chain (World and Ocean).
> > > > >>
> > > > >> For the concrete example above: Maybe there is a spell checker
> > > > >> testcase
> > > > >> which could be bound to the filename glob pattern *.properties.
> > > > >>
> > > > >>> i don't think there's an alternative to a metadata to assist
> > > > >>> mapping
> > > > >>> the
> > > > >>> patch to a relevant "topic" in the code.
> > > > >>> whether it exists as a git note or a label in the commit, that's
> > > > >>> another
> > > > >>> matter and probably less important.
> > > > >> The idea is to use the path/filename and dependency tree information
> > > > >> to
> > > > >> model these topics. Example:
> > > > >> WaterTestsuite(Topic):
> > > > >>    regex_in_change: .*\.fish
> > > > >>    glob_in_change: src/classes/ocean/*.java
> > > > >>    path_in_change: src/classes/water.java
> > > > >>    change_affects_depency_of: WaterClass
> > > > > I'm not familiar that much with the names of the classes and
> > > > > filenames,
> > > > > but
> > > > > it sounds to me like an error prone process
> > > > > and very complex to start going through all the classes and file
> > > > > names
> > > > > and
> > > > > mapping them to a certain project/component.
> > > > > sounds like we'll have to enforce a naming convention for every new
> > > > > file/path/class name that won't break that magic
> > > > > detection.
> > > > >
> > > > > sure there are exceptions that will work probably, like "anything
> > > > > under
> > > > > packaging/, should trigger the 'engine-setup' or 'engine-upgrade'
> > > > > tests,
> > > > > but imo, it is not so easy with other components.
> > > > >
> > > > > if something will help, it will be splitting up ovirt-engine into
> > > > > subject
> > > > > projects (different git)
> > > > >
> > > > > Eyal.
> > > > 
> > > > I think some valid points were raised in this thread, and I feel we all
> > > > agree regarding the need for such a mechanism.
> > > > regarding mapping of different areas in the code using metadata, i
> > > > think
> > > > this approach worth trying, it'll increase ownership and area of
> > > > responsibility within our code and hopefully provide us the
> > > > functionality we are looking for.
> > > > we can start doing the obvious mapping, after-which the responsibility
> > > > of each team/maintainer to assign a file to a person and define the
> > > > specific functional areas in it.
> > > > 
> > > > Moran.
> > > > 
> > > > >
> > > > >> But surely labels or meta-data in the commit msg are quicker to
> > > > >> implement.
> > > > >>
> > > > >> - fabian
> > > > >>
> > > > >>> eyal.
> > > > >>>
> > > > >>>> - fabian
> > > > >>>>
> > > > >>>> _______________________________________________
> > > > >>>> Engine-devel mailing list
> > > > >>>> Engine-devel at ovirt.org
> > > > >>>> http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > >>>>
> > > > >>
> > > > >>
> > > > > _______________________________________________
> > > > > Infra mailing list
> > > > > Infra at ovirt.org
> > > > > http://lists.ovirt.org/mailman/listinfo/infra
> > > > 
> > > > 
> > > _______________________________________________
> > > Engine-devel mailing list
> > > Engine-devel at ovirt.org
> > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > 
> > 
>

More information about the Devel mailing list