-
Notifications
You must be signed in to change notification settings - Fork 273
Change code owners #2587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change code owners #2587
Conversation
Leave code owners for test folders unspecified
a81147c
to
52d71d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. I actually don't have much of an issue with the current status, so whatever others are happy with is ok for me.
CODEOWNERS
Outdated
/.travis.yml @diffblue/devops @thk123 @forejtv @peterschrammel | ||
/appveyor.yml @diffblue/devops @thk123 @forejtv @peterschrammel | ||
# All remaining files are assumed to be low-risk | ||
# Any two reviewers can be assigned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I wouldn't really consider any of the CI configuration files "low risk." Changes to them may have impact on the bills paid. Indeed the CodeBuild configuration files should be added here. (I do agree that /scripts/
is low risk.)
@diffblue/devops and @forejtv Are you okay with being removed as code owners from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR failed Diffblue compatibility checks (cbmc commit: a81147c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79480656
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passed Diffblue compatibility checks (cbmc commit: 52d71d2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/79482358
Personally I think we should leave the owner list as-is (i.e. the underlying #2580 should be merged, but drop this PR), but change the rules for how auto-review requests ought to be handled: When you receive an auto-review request, ASSIGN (cf. request-review-from) two people to do the actual reviewing (possibly yourself, possibly do it right now if it's trivial). The problem behaviour is that people get auto-review requests and ignore by default. If your response to an auto-request is assign reviewers if not already assigned then the problem goes away. |
As explained in #2580 this should not be merged in its current form. Removing the default code owners has unintended consequences. |
/src/xmllang/ @kroening @tautschnig @peterschrammel | ||
/src/nonstd/ @smowton @peterschrammel | ||
/src/solvers/cvc @martin-cs @kroening | ||
/src/solvers/flattening @martin-cs @kroening @tautschnig @peterschrammel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/src/solvers/cvc is gone
/src/goto-programs/ @kroening @tautschnig | ||
/src/util/ @kroening @tautschnig | ||
/src/solvers/refinement @romainbrenguier @peterschrammel | ||
/jbmc/src/java_bytecode/ @peterschrammel @smowton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't workable without someone from the Deeptest group as a code owner for java_bytecode
as it needs to be changed frequently.
There doesn't seem to be enough support for reducing the number of code owners for each folder. Closing this PR. |
This is based on top of #2580. This PR only concerns the last two commits. See the old PR for more discussion.