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

Fri Aug 23 10:00:38 UTC 2013

----- Original Message -----
> From: "Alon Bar-Lev" <alonbl at redhat.com>
> To: "David Caro" <dcaroest at redhat.com>
> Cc: "Eyal Edri" <eedri at redhat.com>, "engine-devel" <engine-devel at ovirt.org>, "infra" <infra at ovirt.org>
> Sent: Friday, August 23, 2013 10:45:37 AM
> Subject: Re: [Engine-devel] Proposal for new commit msg design for engine commits
> 
> 
> 
> ----- Original Message -----
> > From: "David Caro" <dcaroest 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: Friday, August 23, 2013 11:16:31 AM
> > Subject: Re: [Engine-devel] Proposal for new commit msg design for engine
> > commits
> > 
> > On 07/20/2013 08:53 PM, Alon Bar-Lev wrote:
> > > 
> > > 
> > > ----- 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.
> > 
> > I suggest using a tag at the end with some extra syntax, like:
> > Components: core, storage, db
> > Components: all
> > Components: all, !core, !db
> > 
> > > 
> > > For example, database change should trigger what?
> > All the jobs that are tagged for that component (db upgrades I suppose).
> > And if the changes affect storage components then also storage, if the
> > changes affect others then those others too.
> > 
> > > Infra change should trigger what?
> > The same, all the jobs that are tagged as infra.
> > 
> > > A change of both user interface and command should trigger what?
> > All the jobs tagged by user interface and/or command.
> > 
> > > So you end up with:
> > > 
> > > userportal: storage: core: db: some message
> > 
> > As I suggested before, I think it's better if you end with a commit
> > message like:
> > 
> >    Some message
> > 
> >    Components: userportal, storage, core, db
> > 
> > Actually it can be easily done with a tag in the gerrit comment instead
> > of the commit message.
> > 
> > > 
> > > Just to make who happy?
> > 
> > The developer, the qe, the ci and the infra people. This mechanism is to
> > avoid running all the tests all the time. Of course there are some times
> > when all the tests should be run to make sure nothing else changed, but
> > most times you just need to run part of them to make sure you did not
> > break something obvious.
> > 
> > > 
> > > 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?
> > 
> > As I said before, what the developer specifies is not a list of tests,
> > but a list of components, that qe has to map to different sets of tests
> > that can change with time. So specifying webadmin will run all the tests
> > in that group, that might be only one, or 100, and might be
> > increasing/decreasing with time transparently for the developer. Adding
> > a new component is not common and there's no need to do it so frequently.
> > 
> > > 
> > > Why should it be the developer responsibility and not the quality
> > > ensuring
> > > engineer responsibility to determine which tests should run and when?
> > 
> > Of course it's the responsibility of the qe engineer to determine when
> > and which tests should be run. But this is meant to be a new tool for
> > the developer not a substitute for the full qe tests, so the developer
> > can easily make sure that he's changes do not break anything obvious
> > before starting the real tests (that will take more time and resources).
> > The developer just adds some metadata so the qe engineer can decide
> > which tests to run per patch, so it's on qe's hand in the end to decide
> > if ignore or not the metadata and which tests to run.
> > 
> > > 
> > > 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).
> > 
> > I agree with that, I think that it should be a tag similar to Change-Id,
> > at the end of the commit message.
> > 
> > > 
> > > 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.
> > 
> > That's just not true. The sources are complicated enough to make two
> > changes in the same file to affect different components. Any reused code
> > is prone to affect multiple components, making it really hard to
> > determine by which changed files which tests to run. And if you go down
> > to the function/class level it's even harder to decide and to maintain.
> > And of course it's not human error free, as the metadata in the
> > files/directories is defined and maintained by a human. And in my
> > opinion is a lot harder to implement and maintain, and a lot less agile,
> > and does not get rid of the human factor.
> > 
> 
> Once again...
> 
> 1. Commit messages are not the place to specify metadata to interact with
> automation, it is a record for future reviewer.

Agree, that's why I suggested triggering it from a comment message. But that
will require the developer another step after pushing.

> 
> 2. The metadata within sources are the mean to automate the list of tests
> related to a specific source without human interaction on each commit.

The drawbacks are:
 - It needs a lot of maintenance
 - It require very modular code and 
 - Locks the developer on which tests he want to run

> 
> 3. If there is doubt from list of tests run them all, this is simple rule for
> automation.

Of course, but if there is too much doubt all that maintenance is useless.

> 
> 4. The metadata within files are not the only way to order tests, one can do
> this manually via jenkins or any other mean as one can now.

As with any other solution.

> 
> 5. The metadata within files will help us to achieve other targets, such as
> automatic CC maintainers, verify that +2/-2 are set by authorized maintainer
> of component etc...

That's true, but I think that maybe putting metadata in each file is overkilling.
One file in the root of the repo with a few lines should be enough for that afaik.

> 
> 6. Gerrit 2.6 supports labels[1] which are actually what you are trying to
> achieve using the commit message because of lack of other solutions.

They are not fit for that, at least yet.

> 
> Until we have metadata within source, and we don't as we void discuss this
> for long time, and try to find manual workarounds and solutions.

Or until we have tags in the messages, for the same reasons. Don't forget that
metadata within source is also a 'manual workaround or solution', as someone has
to maintain all the metadata in all the files/directories (maybe for that purpose
will be better to have just one file with all that metadata, depending on which
level of granularity we need to map all the files).

> 
> Add label for each test, this will allow ordering tests via the gerrit web
> interface post submit.

Adding a comment will too.

> 
> After people start to do this over and over and over and over they will
> appreciate the need to add metadata into the source tree.

I think that having to maintain metadata in all the files is way more annoying,
but yes, as you say adding metadata to each file gives a lot more information,
but only if it's up to date.

> 
> In future, if this works out we can help gerrit to improve by enhancing the
> labels into free text/combo box etc...
> Or maybe try to do this now if you like to help out gerrit to improve...

That will make labels fit our purpose, and avoid having to add tags, metadata
and all that, but it's not done yet. I'll gladly add that feature, but I'm not
familiar with java or gerrit code, so I might not be the best person to do it,
I'm sure that it will take a lot more time for me than the other options (Eyal,
any comment?). If you are fluent with java and you want to help you are very
welcome :)

David