~alpine/devel

10 7

[alpine-devel] How to improve quality control for patch reviews

Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw>
Sender timestamp
1532607358
DKIM signature
missing
Download raw message
Hi!

We have had a few complaints from Jakub Jirutka about qualtiy control.

A recent example is
https://github.com/alpinelinux/aports/commit/6c21984c5b86ca03c72a88927f8c1de0c420e25f#commitcomment-29834072

We do struggle with keep up with the PR queue and at the same time do
good enough quality control. I also think we may have different
opinions what is good enough.

I wonder if you have any ideas how we deal with this?

A few things I think we can do:

- improve documentation. Write documentation with a simple checklist
  you can look over before you submit a PR. For example, "check that
  license is in SPDX format[1]", "check that it does not automatically
  start services from pre-install", etc. This will make it easier for
  people doing patch reviews and can be useful when adding automated
  checks.

- add more automatic checks

- give more people push access. Look for people that are candidates to
  get push access. Help them to improve. Follow up when they are "good
  enough".


Any other ideas or thoughts?

-nc

1: https://spdx.org/licenses/


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Details
Message ID
<20180726122719.GB16298@homura.localdomain>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532608040
DKIM signature
missing
Download raw message
Hey Natanael, I haven't been active much recently but I do have some
thoughts here.

On 2018-07-26  2:15 PM, Natanael Copa wrote:
> - improve documentation. Write documentation with a simple checklist
>   you can look over before you submit a PR. For example, "check that
>   license is in SPDX format[1]", "check that it does not automatically
>   start services from pre-install", etc. This will make it easier for
>   people doing patch reviews and can be useful when adding automated
>   checks.

Docs are great but linters are much better (something you alluded to).

> - add more automatic checks

I'm working on a service, sr.ht[0], which provides a number of services
including mailing lists[1] and continuous integration[2]. One feature I
want to support is creating CI builds from patches sent to mailing lists
(including off-site lists), and I already have a thing which can wire up
GitHub pull requests to CI builds.

[0] https://sr.ht
[1] https://lists.sr.ht/~sircmpwn
[2] https://builds.sr.ht/job/5448

These builds could run in a fresh Alpine install (full virt with KVM)
and attempt to build the package, catching any missing makedeps or
ghostly dependencies like the example you gave suffered from, run a
linter, and report the results back to the ML or the GitHub PR.

Would something like that be useful for Alpine?


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Chris Ely <tcely@icloud.com>
Details
Message ID
<67E3C4D9-0183-45E7-8381-662CD7F54C4B@icloud.com>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532636851
DKIM signature
missing
Download raw message
> On Jul 26, 2018, at 8:15 AM, Natanael Copa <ncopa@alpinelinux.org> wrote:
> 
> Any other ideas or thoughts?

I'd like to see more use of collaborators feature of GitHub for aports specifically. I sent my ideas for this in an earlier email. With a bit of automation we can turn GitHub into a sort of staging area where people can take a more active role before being given push access. As of right now, there's very little contributors can do with PRs as they aren't part of the alpinelinux organization.

I personally find having stale labels / reviews and no feedback very frustrating. So I'm very much in favor of expanding the pool of people who can provide feedback and adjust labels / review statuses.

---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20180726170517.670acc18@ncopa-desktop.copa.dup.pw>
In-Reply-To
<20180726122719.GB16298@homura.localdomain> (view parent)
Sender timestamp
1532617517
DKIM signature
missing
Download raw message
On Thu, 26 Jul 2018 08:27:20 -0400
Drew DeVault <sir@cmpwn.com> wrote:

> Hey Natanael, I haven't been active much recently but I do have some
> thoughts here.
> 
> On 2018-07-26  2:15 PM, Natanael Copa wrote:
> > - improve documentation. Write documentation with a simple checklist
> >   you can look over before you submit a PR. For example, "check that
> >   license is in SPDX format[1]", "check that it does not automatically
> >   start services from pre-install", etc. This will make it easier for
> >   people doing patch reviews and can be useful when adding automated
> >   checks.  
> 
> Docs are great but linters are much better (something you alluded to).

I was thinking docs could be helpful when implementing linter.
 
> > - add more automatic checks  
> 
> I'm working on a service, sr.ht[0], which provides a number of services
> including mailing lists[1] and continuous integration[2]. One feature I
> want to support is creating CI builds from patches sent to mailing lists
> (including off-site lists), and I already have a thing which can wire up
> GitHub pull requests to CI builds.
> 
> [0] https://sr.ht
> [1] https://lists.sr.ht/~sircmpwn
> [2] https://builds.sr.ht/job/5448
> 
> These builds could run in a fresh Alpine install (full virt with KVM)
> and attempt to build the package, catching any missing makedeps or
> ghostly dependencies like the example you gave suffered from, run a
> linter, and report the results back to the ML or the GitHub PR.
> 
> Would something like that be useful for Alpine?

I think this sounds like something we are looking for, but it may be
a bigger bigger question related. See
https://bugs.alpinelinux.org/issues/9134

-nc


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Chris Ely <tcely@icloud.com>
Details
Message ID
<DD83B753-1812-4ADB-9CB9-7340BD633FE8@icloud.com>
In-Reply-To
<20180726225421.3be824d6@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532640434
DKIM signature
missing
Download raw message
> On Jul 26, 2018, at 4:54 PM, Natanael Copa <ncopa@alpinelinux.org> wrote:
> 
> I'd also like to a bot that can set a tag on issues that has responded to
> feedback.

https://probot.github.io/apps/no-response/ <https://probot.github.io/apps/no-response/>

Here is a bot along those lines. It's setting a responseRequired label and automatically removing it when the PR opener responds.
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20180726225421.3be824d6@ncopa-desktop.copa.dup.pw>
In-Reply-To
<67E3C4D9-0183-45E7-8381-662CD7F54C4B@icloud.com> (view parent)
Sender timestamp
1532638461
DKIM signature
missing
Download raw message
On Thu, 26 Jul 2018 16:27:31 -0400
Chris Ely <tcely@icloud.com> wrote:

> > On Jul 26, 2018, at 8:15 AM, Natanael Copa <ncopa@alpinelinux.org> wrote:
> > 
> > Any other ideas or thoughts?  
> 
> I'd like to see more use of collaborators feature of GitHub for
> aports specifically. I sent my ideas for this in an earlier email.

I missed this. Sorry.

> With a bit of automation we can turn GitHub into a sort of staging
> area where people can take a more active role before being given push
> access. As of right now, there's very little contributors can do with
> PRs as they aren't part of the alpinelinux organization.

This sounds very interesting. I will followed your other email.

> I personally find having stale labels / reviews and no feedback very
> frustrating. So I'm very much in favor of expanding the pool of
> people who can provide feedback and adjust labels / review statuses.

Yes, this is something I want fix.

I think that we could probably start with a few bots that creates
labels based on :+1: votes. It would be very helpful to be able to sort
PRs on number of upvotes.

I'd also like to a bot that can set a tag on issues that has responded to
feedback.

-nc


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Jean-Louis Fuchs <ganwell@fangorn.ch>
Details
Message ID
<1532897225.933218.1456675080.62DDC693@webmail.messagingengine.com>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532897225
DKIM signature
missing
Download raw message
Hi

On Thu, Jul 26, 2018, at 05:15, Natanael Copa wrote:
> - give more people push access. Look for people that are candidates to>   get push access. Help them to improve. Follow up when they are "good>   enough".

In theory you don't need push access to review a PR:

- If there is a problem anybody can state it on github or the
  mailinglist- If one thinks the PR is ok one can write "looks good to me"

Idea: maybe it is possible that more people are allowed to add a label: "looks good", but without push access. So people with push access can work faster.
Best,
Jean-Louis
Carlo Landmeter <clandmeter@gmail.com>
Details
Message ID
<CA+cSEmN91Cx9sQQg=Lk=_ppVVB4R49J9R-JEHwZjmeUDAYAm8Q@mail.gmail.com>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532936315
DKIM signature
missing
Download raw message
hi Natanael,
>
>
> - improve documentation. Write documentation with a simple checklist
>   you can look over before you submit a PR. For example, "check that
>   license is in SPDX format[1]",


This check has already been implemented. `apk add spdx-licenses-list`
should enable it.
We probably have to move this to main and make it a dep in abuild.

>
> "check that it does not automatically
>   start services from pre-install", etc. This will make it easier for
>   people doing patch reviews and can be useful when adding automated
>   checks.

We could add checks for this, but I don't see this very often so i
guess is case specific?

>
>
> - add more automatic checks


nmeum has started coding an abuild linter in go. He got some negative
remarks as it was
not written in some peoples favorite language which maybe stopped him
from ever finishing it?

>
>
> - give more people push access. Look for people that are candidates to
>   get push access. Help them to improve. Follow up when they are "good
>   enough".


The PR list is mostly populated by a handful of people. I think it
would be good if we look
at giving one or two commit access and keep an eye on their
contributions. It will save us
some time having to review simple contributions.

>
>
> Any other ideas or thoughts?
>

Regarding documentation, it think this is a task you (ncopa/fabled)
has to take on from your side.
Don't get me wrong, I'm not trying to criticize negatively but I think
its important that more
experienced people should show what Alpine as a project expects from a
given product/project
that extends to Alpine experience/functionality. Two major corner
stones of our project (build tools
and package manager) are heavily under documented and have been addressed as
"Read the source" too many times.
If infra team has to provide such facility to start creating
documentation this would be
no problem, but it does need follow up so it doesn't end up as a waste
of infra time.

-carlo


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Chris Ely <tcely@icloud.com>
Details
Message ID
<AACD560B-34B5-4C8F-BAB1-D2B21A324597@icloud.com>
In-Reply-To
<1532897225.933218.1456675080.62DDC693@webmail.messagingengine.com> (view parent)
Sender timestamp
1532978101
DKIM signature
missing
Download raw message
> On Jul 29, 2018, at 4:47 PM, Jean-Louis Fuchs <ganwell@fangorn.ch> wrote:
> 
> Hi
> 
> On Thu, Jul 26, 2018, at 05:15, Natanael Copa wrote:
>> - give more people push access. Look for people that are candidates to
>>   get push access. Help them to improve. Follow up when they are "good
>>   enough".
> 
> In theory you don't need push access to review a PR:
> 
> - If there is a problem anybody can state it on github or the mailinglist
> - If one thinks the PR is ok one can write "looks good to me"

The problem I've seen with this approach is that PRs don't change state until someone with "write" access to the aports repository on GitHub comes along. If the set of people with  "push" access to git.alpinelinux.org is relatively small and the subset of those people with write access on GitHub is even smaller, I don't see how keeping up with aports changes is going to remain possible.

> Idea: maybe it is possible that more people are allowed to add a label: "looks good", but without push access. So people with push access can work faster.

This is the idea behind pull request approvals. I've suggested using https://github.com/zalando/zappr for this in a previous email to this list.

We'll need to determine the best way to expand the pool of of people who can change a PRs labels / assignments, etc. on GitHub. I think collaborators is a good way, but that hasn't been tested yet, as far as I know. Ultimately, an accepted PR can't really be "complete" until it gets merged into master back on git.alpinelinux.org. Expanding the number of people with "push" access to git.alpinelinux.org might just be unavoidable here as the amount of work increases with every new active software package.

---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Details
Message ID
<b4ce1c24-a8e7-4160-05fc-bf2c6ecb0ee6@bitmessage.ch>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1532993400
DKIM signature
missing
Download raw message
Hey everyone,

I had looked into CI quality improvement tools for aports at some point,
and found these:
* shellcheck: catches lots of shell errors, can be set to POSIX shell
* shfmt: could be used to enforce a certain syntax in the shell (similar
  to PEP8 for Python)

(we're using shellcheck in pmbootstrap/aports for post-install scripts
so far, not for APKBUILDs yet, but that should also work)

The catch is, that both are not packaged for Alpine yet.

Another point where automation could help is soname breakage: sometimes
one package get updated, but other packages depending on it do not get a
pkgrel bump. The APKINDEX file already contains information which
programs depend on which sonames, so it is possible to figure out what
will break. We have such a CI job for postmarketOS packages in
pmbootstrap (written in Python), and if there's interest, I could port
it so it works independently in Alpine's aports repository. (I'm pretty
busy right now, so it will take me a while though.)

Thanks,
Oliver

Natanael Copa:
> Hi!
> 
> We have had a few complaints from Jakub Jirutka about qualtiy control.
> 
> A recent example is
> https://github.com/alpinelinux/aports/commit/6c21984c5b86ca03c72a88927f8c1de0c420e25f#commitcomment-29834072
> 
> We do struggle with keep up with the PR queue and at the same time do
> good enough quality control. I also think we may have different
> opinions what is good enough.
> 
> I wonder if you have any ideas how we deal with this?
> 
> A few things I think we can do:
> 
> - improve documentation. Write documentation with a simple checklist
>   you can look over before you submit a PR. For example, "check that
>   license is in SPDX format[1]", "check that it does not automatically
>   start services from pre-install", etc. This will make it easier for
>   people doing patch reviews and can be useful when adding automated
>   checks.
> 
> - add more automatic checks
> 
> - give more people push access. Look for people that are candidates to
>   get push access. Help them to improve. Follow up when they are "good
>   enough".
> 
> 
> Any other ideas or thoughts?
> 
> -nc
> 
> 1: https://spdx.org/licenses/
> 
> 
> ---
> Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
> Help:         alpine-devel+help@lists.alpinelinux.org
> ---
> 
> 



---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Timo Teras <timo.teras@iki.fi>
Details
Message ID
<20180802110635.20e1f810@vostro>
In-Reply-To
<20180726141558.2d451763@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1533197195
DKIM signature
missing
Download raw message
On Thu, 26 Jul 2018 14:15:58 +0200
Natanael Copa <ncopa@alpinelinux.org> wrote:

> We do struggle with keep up with the PR queue and at the same time do
> good enough quality control. I also think we may have different
> opinions what is good enough.
> 
> I wonder if you have any ideas how we deal with this?

We need to automate more things.

> - add more automatic checks

This is the key. We need good linter that catches issues. It should be
CI hook in github to audit changes and give automated feedback.

The increasing amount of rules (some written, some not) are sometimes
missed. Especially when the policy changes and people forget which is
the latest direction. We need linter that is the authority and keep it
up-to-date.

Thanks,
Timo


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---
Reply to thread Export thread (mbox)