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

[Alon Bar-Lev]

----- Original Message -----
> From: "David Caro Estevez" <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 1:00:38 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: "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

One time maintenance compared to per patch.
Easier to enforce project policy, than relay on developer's policy.

>  - It require very modular code and

Right, we require this anyway.

>  - Locks the developer on which tests he want to run

As I wrote, it locks nothing, it is the baseline.

> > 
> > 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.

So you suggest we run partial? based on whose decision? can we trust him?
 
> > 
> > 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.

Right, because of that I do not think this is emergency.
Just have jenkins jobs and people can order tests based on gerrit urls...
 
> > 
> > 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.

The metadata model I suggested was within each source and within directory.
The collection is metadata of source + recursive meta data of directories.

At file put signature.
At directory put a file, such as .ovirt.metadata with same fields.

> > 
> > 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.

So maybe better if we can help gerrit to improve[1]?

[1] http://code.google.com/p/gerrit/issues/detail?id=2085

> > 
> > 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).

I think best is to spread metadata within the entire tree, as then maintainer of file control the metadata, if file is copied/duplicated/moved/renamed it keeps the metadata.

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

So why relay on commit message, you can just ask people to comment:

OVIRT_ORDER_TEST: test1
 
> > 
> > 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.

Just like claiming that the source is out of date.
If there is a mistake in metadata the maintainer will detect that when metadata is used.
 
> > 
> > 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 :)

You can help in making the metadata into the tree.
This will be a great step forward.

Thanks,
Alon