Thanks a lot Otavio, for bringing this up!

I can only second Nicolas that CI should not be circumvented unless there is a serious reason. Second, PRs should ideally contain one compilable commit per logical change. As long as there is a just a single such commit in a PR, then the CI can work as a good barrier against broken commits.

Otherwise it is up to the PR author to make sure that the individual commits are compilable too. I personally always try honor this rule. If my PR contains multiple commits, I often take care to re-order and/or fixup via git rebase --interactive. The readability of the history and the time of my co-workers matter to me.

Thanks,

-- Peter

On 22/06/2023 10:12, Nicolas Filotto wrote:
Hi,

For me, the best you can do to avoid this kind of problem is to avoid committing directly into the 
target branch by always submitting a PR for any change. And then, once the build is green, merge 
the PR with "Squash and merge" which is the default action on Camel. The action 
"Rebase and merge" should be avoided since the build result reflects the final state of 
the branch, not the intermediate states. In case you need some changes to fix the build, make sure 
to re-synchronize the source branch with the target branch, especially in case the changes are not 
done on the same day because the Camel branches are modified very frequently and each change can 
potentially be in conflicts with yours.

it is a quite cumbersome approach but I don't see any safer way.

Regards,
Nicolas
________________________________
From: Otavio Rodolfo Piske <angusyo...@gmail.com>
Sent: Thursday, June 22, 2023 08:53
To: dev@camel.apache.org <dev@camel.apache.org>
Subject: Re: Some thoughts on bisecting Camel and our git commit practices

Hi,

Release-tagged commits don't help here. The whole point of bisecting is
finding out precisely which commit introduced a certain behavior or bug, so
that I and other committers can diagnose, document and fix the problem.
It's not possible to do that effectively with just release-tagged commits.

Also, I don't think each commit needs to be "release quality": that would
be too much right now. I do think, however, that each commit should be
compilable. Consider, for instance, the following commit pattern I notice
when bisecting the code:

commit1: TASK111: did some stuff  ---> not compilable
commit2: TASK111: did some stuff  ---> compilable
commit3: TASK111: did some stuff ---> not compilable
commit4: TASK111: did some stuff ---> compilable

Git bisect run is smart enough to skip commit2 and commit3 if given a
proper exit code (that's what we do). However, the problem gets worse once
these compilable scripts start to pile up, such as:

commit1: TASK111: did some stuff  ---> not compilable
commit2: TASK111: did some stuff  ---> compilable
commit3: TASK111: did some stuff ---> not compilable
commit4: TASK111: did some stuff ---> compilable
commit5: TASK222: did some stuff  ---> not compilable
commit6: TASK222: did some stuff  ---> compilable
commit7: TASK222: did some stuff ---> not compilable
commit8: TASK222: did some stuff ---> compilable

At some point, the bisect execution runs out of skippable commits and you
have to: a) test manually, b) restart the bisect operation with different
good/bad commits, or c) investigate the list of skipped commits one by one.

The same can happen when merging community contributions composed of
multiple commits.

So, I think we can improve here by doing 2 things:

1. When commiting the work on a task, making sure that every commit is
compilable. For instance, in the pattern above, by squashing
commit1+commit2, commit3+4, etc.
2. When merging community contributions with multiple PRs, squashing the
contribution before merging - if suspicious that one or more of the commits
introduce uncompilable changes.

If we start doing this now, bisecting will be much easier in the future.

Kind regards

On Thu, Jun 22, 2023 at 8:08 AM Petr Kuzel <petrku...@eurofins.com.invalid>
wrote:

Hi Otavio,

the codebase does not have the release quality
after each commit. Test-driven development could
even invite devs to separately commit failing tests
as a proof they are effective.

On contrary the release-tagged commits should
be safe. Would not help you to focus
on the release-tagged commits only, pls?


   Hope it helps
   Cc.

--
   Mr. Petr Kužel, Software Engineer
   Eurofins International Support Services s.à r.l.
   Val Fleuri 23
   L-1526 LUXEMBOURG

-----Original Message-----
From: Otavio Rodolfo Piske <angusyo...@gmail.com>
Sent: Wednesday, June 21, 2023 11:17
To: dev <dev@camel.apache.org>
Subject: Some thoughts on bisecting Camel and our git commit practices


Folks,

You may have seen that PRs constantly report failures when testing changes
on core. This is almost always caused by flaky tests.

I have been trying to find the root causes of those flaky tests. Quite
often this involves going all the way to early versions of Camel 3 to find
stable executions of those tests.

The investigation itself is mostly automated by using git bisect run ...
This significantly improves the developer ability to find the root causes
of issues.

Despite all this automation, however, the task has been ... quite difficult
and lengthy. Even considering the extensive resources I have access to
(thanks to my employer), bisecting a single test failure can take several
*days*.

In particular, this investigation has been difficult because we have a huge
amount of dirty commits on the tree: partially modified work, uncompilable
changes, broken tests and more.

While it's NOT my intention to suggest introducing/discussing a full blown
clean commit policy, I do wonder if we have some room to discuss how we can
improve the way we work to ensure that the tree is compilable and somewhat
testable (and, possibly, enforce that).

We cannot fix the git history of the project, but we can ensure that future
investigation can be made much easier in the future.

So, folks, does anyone have any suggestions about how we could improve
here?

Kind regards
--
Otavio R. Piske
https://urldefense.com/v3/__http://orpiske.net/__;!!CiXD_PY!SZvpton5vuSh5elDATUqZmerIhuL7ZGmoTwQB-66e3u34QRJzR2DoTvCjF3miU2oF4glDqKC5VnT439E0B0$



--
Otavio R. Piske
https://urldefense.com/v3/__http://orpiske.net__;!!CiXD_PY!SZvpton5vuSh5elDATUqZmerIhuL7ZGmoTwQB-66e3u34QRJzR2DoTvCjF3miU2oF4glDqKC5VnTh1-lNIM$

As a recipient of an email from the Talend Group, your personal data will be processed by our systems. 
Please see our Privacy Notice <https://www.talend.com/privacy-policy/> for more information about 
our collection and use of your personal information, our security practices, and your data protection 
rights, including any rights you may have to object to automated-decision making or profiling we use to 
analyze support or marketing related communications. To manage or discontinue promotional 
communications, use the communication preferences 
portal<https://info.talend.com/emailpreferencesen.html>. To exercise your data protection rights, 
use the privacy request 
form<https://talend.my.onetrust.com/webform/ef906c5a-de41-4ea0-ba73-96c079cdd15a/b191c71d-f3cb-4a42-9815-0c3ca021704cl>.
 Contact us here <https://www.talend.com/contact/> or by mail to either of our co-headquarters: 
Talend, Inc.: 400 South El Camino Real, Ste 1400, San Mateo, CA 94402; Talend SAS: 5/7 rue Salomon De 
Rothschild, 92150 Suresnes, France


Reply via email to