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

----- Original Message ----- > From: "David Caro Estevez" <dcaroest(a)redhat.com&gt; > To: "Alon Bar-Lev" <alonbl(a)redhat.com&gt; > Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, "engine-devel" <engine-devel(a)ovirt.org&gt;, "infra" <infra(a)ovirt.org&gt; > 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(a)redhat.com&gt; >> To: "David Caro" <dcaroest(a)redhat.com&gt; >> Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, "engine-devel" >> <engine-devel(a)ovirt.org&gt;, "infra" <infra(a)ovirt.org&gt; >> 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(a)redhat.com&gt; >>> To: "Alon Bar-Lev" <alonbl(a)redhat.com&gt; >>> Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, "engine-devel" >>> <engine-devel(a)ovirt.org&gt;, "infra" <infra(a)ovirt.org&gt; >>> 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(a)redhat.com&gt; >>>>> To: "Alon Bar-Lev" <alonbl(a)redhat.com&gt; >>>>> Cc: "infra" <infra(a)ovirt.org&gt;, "engine-devel" >>>>> <engine-devel(a)ovirt.org&gt;, >>>>> "Fabian Deutsch" <fabiand(a)redhat.com&gt; >>>>> 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