CI system maintainability

Thu Mar 28 15:45:51 GMT 2019

On Thursday, March 28, 2019 3:39:54 PM CET Friedrich W. H. Kossebau wrote:
> 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?

I wrote this with my PIM hat on and targeted the PIM community, without 
realizing this is a cross-post, so I apologize for the confusion. I believe 
each project should choose whatever workflow and policy suits them.

That said, I do put up for review everything that is not Akonadi/PIM core 
(even changes to PIM components that are not "mine"). The reason I don't do 
this for Akonadi is that there's no-one really to review my code, because no-
one else works on it, or at least that has been my perception of the situation 
lately.

I've been thinking about bringing this topic up on the upcoming PIM sprint as 
I would like to have my code reviewed, I think it's a very good and healthy 
thing, but in case of KDE PIM, I think we need to agree that we'll actually 
review each others code, even if it's not "our own" code-base.

> Also, how do you ensure the review is actually of quality?

How do you ensure that the code people commit without a review is 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.

This is a fair point, and I certainly am guilty of doing this occasionally in 
the past. But even reviewer can have a reviewer: if you keep accepting patches 
of "dubious quality" (break build, don't work, contain typos ...), someone 
else from the community will eventually notice and point out you should maybe 
be more careful with your reviews or pay attention to certain aspects (like 
"does it actually work?).

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

AFAIU the policy right now is keep putting up patches for review until you get 
commit access, then continue doing so until you feel like you don't have to 
anymore. 

But having experienced contributors putting up their code for review is a good 
thing as it allows newcomers (and not just them!) to better understand what is 
happening in the project and simple reviews can be a good starting point for 
them to get more involved in the community.

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

This I think is somewhat related to the tooling as I said in my previous 
email. If your workflow for simple obvious fix would be "git push HEAD:refs/
for/master" instead of "git push master", you wouldn't care. If that means 
using arc, I can understand....

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

Yep, review builds are not a silver bullet, but are a huge improvement - for 
instance in most cases I only run a limited subset of Akonadi tests before 
pushing, simply because on my laptop they take forever to run. If the CI can 
run all of them for me in a fully isolated environment (while I move on to 
work on another bugfix), it's a /major/ improvmenet and for me yet another 
reason to put my code up for review.

 - Dan

> 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

-- 
Daniel Vrátil
www.dvratil.cz | dvratil at kde.org
IRC: dvratil on Freenode (#kde, #kontact, #akonadi, #fedora-kde)

GPG Key: 0x4D69557AECB13683
Fingerprint: 0ABD FA55 A4E6 BEA9 9A83 EA97 4D69 557A ECB1 3683
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20190328/2849976a/attachment.sig>