Don't use cbmc-developers as a code owner #2580
Conversation
owen-mc-diffblue
requested review from
chrisr-diffblue,
kroening,
peterschrammel,
smowton and
tautschnig
as code owners
|
"The people you choose as code owners must have write permissions for the repository." |
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: 02340c2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79097539
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
|
To be clear, I'm not sure exactly what syntax GitHub want for "empty list", so there is a chance that merging this will stop code owners for working at all. We should therefore merge it with caution and watching following PRs to check everything works smoothly. |
CODEOWNERS
Outdated
| unit/ | ||
| regression/ | ||
| jbmc/unit/ | ||
| jbmc/regression/ |
There was a problem hiding this comment.
Shouldn't those lines be removed completely, together with the comment above it?
There was a problem hiding this comment.
Removing the lines would have the effect of making those folders only match *, so the six people listed there would be code owners. The effect I'm trying to get is to make no one a code owner of those folders. How github will interpret this PR isn't very well documented, and the only way to find out is to try it. It's also possible I have the wrong syntax - they say it follows .gitignore, which is related to gitattributes.
There was a problem hiding this comment.
Could a group enlisting exactly those with write access be set up? Relying on undocumented or poorly documented behaviour is just bound to cause confusion.
There was a problem hiding this comment.
I think we actually want to keep the current behaviour, where no one is assigned as code owner for PRs that only edit those folders. The intention is to say "These files are so low risk that anyone with write access can approve them."
I have added an extra comment to this PR making it more explicit what it is doing.
This PR relies on poorly documented behaviour less than the previous state of affairs (assigning a group without write access to be code owners). I don't think that we shouldn't use an empty list of code owners just because the meagre documentation doesn't explicitly state that you can do that.
|
I note that #2525, which only changes tests, seems to not to have requested reviews from any code owners, supporting my assumption that this was what happened when a group without write access was in the code owners file. |
owen-mc-diffblue
force-pushed
the
owen-jones-diffblue/update-codeowners
branch
from
02340c2 to
931ba3f
Compare
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: 931ba3f).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79136180
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
peterschrammel
force-pushed
the
owen-jones-diffblue/update-codeowners
branch
from
931ba3f to
ffbbf74
Compare
|
@owen-jones-diffblue, @smowton, a couple of new suggestions to reduce spamming with review requests. Look at individual commits. |
|
Hmmmm, worth a try. It might impede people a bit though that e.g. for Java either you or I have to approve. Really we do want many owners, but for the PR submitter or triage person to select particular reviewers... |
|
Are there other (admin?) permissions that make it possible to occasionally override the codeowners requirement? (So, we could have two codeowners who generally review a specific part, but other people who could review and merge when the two codeowners are not available.) |
| @@ -1,6 +1,3 @@ | |||
| # These owners will be the default owners for everything in the repo. | |||
| * @kroening @tautschnig @peterschrammel @smowton @chrisr-diffblue | |||
There was a problem hiding this comment.
(Removing the default codeowners could go it a separate commit, rather than one labelled "Don't use cbmc-developers as a code owner".)
There was a problem hiding this comment.
Yeah; this does seem like quite a change in the workflow.
There was a problem hiding this comment.
I've split this into a separate commit
There was a problem hiding this comment.
Passed Diffblue compatibility checks (cbmc commit: ffbbf74).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79447627
|
@peterschrammel This solution is fine with me, and is definitely an improvement on the status quo. Ideally we'd like to do slightly more than is possible with the code owners mechanism, like requesting reviews from 2 randomly selected people out of a list of 10 that know those files. Github recommend Probot as a framework for writing apps to do this kind of thing. |
|
Suggestion: how about instead of reducing the pool of people authorised to review a particular PR (i.e. the reduction to two people per directory in the last commit), they should serve as triage people? Their responsibility is to ASSIGN people, whether owners or not, to do the actual reviewing. If the reviewers are non-owners, they also need to give final approval (but this can mean just checking they in turn approve). This should mean only triage people receive unnecessary email, and they should always respond to that by (a) checking that review requests have been made, or if not make them, then (b) muting the thread until further notice. Their attention can of course be attracted by |
|
If you are assigned to review but can't for some reason, it's your job to hand the task off to someone appropriate. |
owen-mc-diffblue
force-pushed
the
owen-jones-diffblue/update-codeowners
branch
from
ffbbf74 to
c07e458
Compare
|
I think I agree with @smowton 's issue (not sure how I feel about the proposed solution). It's not clear to me why two owners is the right number; why not three, why not the 2-4 number we had previously? |
There was a problem hiding this comment.
Passed Diffblue compatibility checks (cbmc commit: c07e458).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79466148
Everyone is always invited to review a PR.
I don't see the opposition here. Reducing the number of code owners reduces the number of people who get spammed with automatic review requests. Delegating reviews is always possible. The code owner feature is unfortunately very unflexible in this respect and doesn't allow you to unassign code owners from review requests.
Exactly. |
|
I think @smowton is in favour of this PR and is discussing the expectation on code owners: are they expected to review most of the PRs they are requested to or are they expected to request reviews from others and then unfollow, ignoring the PR till it's approved and ready to merge. |
|
In the end, as @martin-cs noted, it's a social problem that requires social solutions (that should be technically facilitated). If we write a GithubApp for managing review automatic review assignments, we could perhaps integrate a refined version of @tautschnig's 2 reviews per submitted PR idea: PR authors give appreciation to their reviewers by paying them a chosen amount of diffcoins for the review work they have done... |
owen-mc-diffblue
force-pushed
the
owen-jones-diffblue/update-codeowners
branch
from
c07e458 to
8ecf23a
Compare
CODEOWNERS
Outdated
| @@ -45,13 +45,6 @@ src/cpp/ @kroening @tautschnig @peterschrammel | |||
|
|
|||
| # These files change frequently and changes are low-risk | |||
|
|
|||
| src/util/irep_ids.def @diffblue/cbmc-developers | |||
There was a problem hiding this comment.
The comment above this line must be removed as well. It does not refer to the bottom three lines that remain.
|
After some discussion with @johnnonweiler I've decided to split this PR into two parts. The part that remains in this PR will implement what I originally intended, but in the better way that @peterschrammel implemented. The second part #2587 is about the extra changes that @peterschrammel added: reducing the number of code owners and removing all code owners from dev-ops files. |
Leave code owners for test folders unspecified
owen-mc-diffblue
force-pushed
the
owen-jones-diffblue/update-codeowners
branch
from
8ecf23a to
4096c92
Compare
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: 8ecf23a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79479722
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
There was a problem hiding this comment.
Passed Diffblue compatibility checks (cbmc commit: 4096c92).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79481535
|
On Thu, 2018-07-19 at 08:37 -0700, Peter Schrammel wrote:
In the end, as @martin-cs noted, it's a social problem that requires
social solutions (that should be technically facilitated). If we
write a GithubApp for managing review automatic review assignments,
we could perhaps integrate a refined version of @tautschnig's 2
reviews per submitted PR idea: PR authors give appreciation to their
reviewers by paying them a chosen amount of diffcoins for the review
work they have done...
@peterschrammel Thinking out loud; maybe this will help...
Why review?
A. As a general QA metric.
B. To avoid technical debt.
C. To enforce coding style(?)
D. Parts of the code are complex and subtle and the design, details,
invariants, know-how, applications, consequences and out-of-tree patch
sets are, unfortunately, only in the heads of various experienced
developers.
The problem:
*. It's the same few people doing reviews and they feel overloaded.
It seems that this might be for one or more of the following reasons:
1. There simply aren't enough people reviewing.
(don't have opportunity, don't have the skills, don't feel engaged /
incentivised, don't feel it is their responsibility, etc.)
2. There simply aren't enough people doing D. style reviewing.
(don't have opportunity, don't have the skills, don't feel engaged /
incentivised, don't feel it is their responsibility, etc.)
3. The workload is not evenly distributed between them.
(why do people review the patches they do?)
4. The people doing the reviewing aren't adequately rewarded.
(why do people review at all?)
It seems to me (in the classic "pouring petrol on the fire" review
technique) that without a clear picture of which bits of 1-4 are the
actual root cause, we are unlikely to fix the problem by just
suggesting things. Is this the kind of human process issue which "lead
devs", "scrum / agile masters" are trained to solve?
[ And, as if this wasn't unhelpful enough, some observations:
*. The CPROVER project has historically had a problem with training /
onboarding / documentation.
*. Most of the people listed as maintainers also have other pressing
responsibilities; CEO, CTO, team and project leads, etc.
*. Most of the people listed have a PhD and post-doc experience in a
relevant area. This is a significant advantage but may also indicate a
process failure of some kind. ]
|
|
@martin-cs Thanks a lot for the nice write-up. I would, however, like to add items E. Learning (learning about the code base and coding in general - yes, reading someone else's code is actually useful!) |
|
This discussion should really move to #2587 , which now contains the more controversial changes to code owners. |
|
There is an issue with the current approach of removing the default code owners - There are a number of files and folders that are not explicitly mentioned in the code owners file. We can fix this by explicitly adding the people who were previously default code owners as code owners for those folders. However, with the current approach of only listing things which should have code owners, I can't see a good way of handling More detail: Files in a folder, but not its subfolders, can be included, as explained in this example from the code owners documentation:
So, we could add these lines to the top of CODEOWNERS:
|
|
After discussion with Owen, I've created another PR #2589 to replace this PR. It adds code owners to all the files which were previously owned by the default code owners, and moves |
|
Closing as this is superseded by PR #2589 |
The GitHub documentation says that in CODEOWNERS you should only
specify people with write access. @diffblue/cbmc-developers does not
have write access. So hopefully this PR won't change things.