CI system maintainability

[Friedrich W. H. Kossebau]

Am Donnerstag, 28. März 2019, 11:27:44 CET schrieb Daniel Vrátil:
> I'm completely fine with mandatory code review for everything and I'd be
> happy to have this in PIM. I think the biggest problem in PIM to overcome
> will be finding the reviewers - I dare say I'm currently the only one who
> has at least a little idea about what's going on in Akonadi, so getting for
> instance Laurent to review my Akonadi patches might be hard - same for me
> reviewing Laurent's KMail patches. This will require non-trivial amount of
> effort for all members of the community to gain deeper understanding of the
> other components within the project and a willingness to step up and do a
> code review even if they don't feel 100% comfortable with the code base.
> But I believe that in the long run the benefits clearly out-weight the
> cost.

So why do you not do this already? Why would you only do this is there is a 
policy requiring you to do it?
And why do you think this policy-driven behavioural change will work or is 
needed with everyone else?

Also, how do you ensure the review is actually of quality?
And not just socially driven "+1 for my buddie!", where buddy then also 
mentally shifts part of responsibility to other buddy, because, he, more eyes 
means I do not need to do the full work myself, glitches will be caught be 
them surely! Reviewer found a code formatting issue, done here, review 
happened.
The opposite also has been seen, reviewers feel they need to do "real" review 
and find things, so start to nitpick or to demand wrong changes, wasting 
everyone's time.

For myself I know I will happily have other people review my patches, but 
only if there are qualified people to be expected to do it with the needed 
quality in a reasonable time.

Then, trying to force by that policy other people to become proper specialist 
for certain other code projects to do qualified reviews, actually is a trade-
off. They will have less time for their actual code project, becoming less a 
specialist there (or having time to review other people's contribution to 
that project).
We need more contributors, not force existing contributors to distribute 
their abilities & resources over more projects (and thus having less for 
their actual one).

I am also not aware of many contributors to KDE projects who are not capable 
to make a responsible decision between the few obvious simple fixes and those 
normal changes which better get feedback from others first. If one is unsure 
about one's post-beer late-night quick hack, one will upload it for review, 
no? At least if one's previous late-night commits turned out to be late-night 
mental state impacted.
To deal with those few which seem challenged to do that decision properly, I 
find such a policy for everyone harmful, I know it would impact my 
contribution willingness, when I come across a typo or other simple and 
obvious to fix things. And just leave the garbage around along my current 
working path.

Besides, it will not solve the issue this thread is about, with some people 
not caring (quickly) enough if CI builds fail or if there are regressions in 
tests.

Review builds once implemented and deployed will help there partially as a 
side-effect only, catching some build fails before. But also not if this 
breaks (e.g. due to API/behavioural changes) depending projects, as CI at 
least currently does not rebuild dependent projects (cmp. also T7374).

A society with people doing things only due to policies and not intrinsic 
motivation seems very flawed to me, makes me wonder why people are in there 
given no-one forced them into this society.

https://community.kde.org/Policies/
Commit_Policy#Code_review_by_other_developers has some policies already, 
which should be enough:
1.1 Think Twice before Committing
1.2 Never commit code that doesn't compile
1.3 Test your changes before committing
1.4 Double check what you commit
1.10 Code review by other developers
1.11 Take responsibility for your commits
1.12 Don't commit code you don't understand
Well, perhaps could be amended by an explicit note about keeping CI working.

Enforcing review of any commits IMHO should be only done for people who 
notoriously failed to comply with those existing rules. If we are unable to 
pinpoint those people, talk with them and make them comply or sort out their 
reasons to not yet comply, but instead create as workaround new generic 
policies for everyone, we make this a worse society. And also a less 
effective, with more rubber stamps needed.

Cheers
Friedrich