Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

header updates to clarify use of CODEOWNERS #9426

Merged
merged 2 commits into from
Jan 15, 2018
Merged

header updates to clarify use of CODEOWNERS #9426

merged 2 commits into from
Jan 15, 2018

Conversation

srochel
Copy link
Contributor

@srochel srochel commented Jan 15, 2018 •
edited
Loading

Description

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

Checklist

Essentials

  • [ x] Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • [ x] To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Feature1, tests, (and when applicable, API doc)
  • Feature2, tests, (and when applicable, API doc)

Comments

Change is not complete and needs to be finalized. Discussion ongoing at https://lists.apache.org/thread.html/6d83f8a93d9a1cd152e869caa1e570e570677886dd5c65fc28298322@%3Cdev.mxnet.apache.org%3E
Do not merge until discussion converged.

@srochel
header updates to clarify use of CODEOWNERS
af5b19c 
Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.
@srochel
fixed typo
3182cbe
@bhavinthaker
Copy link
Contributor

Looks good to me.

Please update the development process on Apache MXNet wiki once this file is merged.
https://cwiki.apache.org/confluence/display/MXNET/Development+Process

@piiswrong piiswrong merged commit 63394de into apache:master Jan 15, 2018
cjolivier01
cjolivier01 reviewed Jan 15, 2018
View reviewed changes
CODEOWNERS
# https://help.github.com/articles/about-codeowners/ and
# https://github.com/blog/2392-introducing-code-owners
#
# The first owner listed for a package is considered the maintainer for a package.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One "maintainer"? I strongly oppose this. Who would even decide who that is? This sounds like a recipe for conflict within the community.

CODEOWNERS
# as additional owners to get notified about changes in a specific package.
#
# By default the package maintainer should merge PR after appropriate review.
# A PR which received 2 +1 (or LGTM) comments can be merged by any committer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still maintain that any committer can merge something if they feel they've given it a good review -- this encourages people to learn new parts of the code and allows the project to scale.

The rule as stated above would mean that only a few people would be "allowed" to merge code at all, which sounds more like an internal corporate style and does not promote community, IMHO.

CODEOWNERS
#
# By default the package maintainer should merge PR after appropriate review.
# A PR which received 2 +1 (or LGTM) comments can be merged by any committer.
# In the future we might consider adopting required reviews
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is "we"?

@cjolivier01
Copy link
Member

This was not agreed upon in dev and should be reverted

piiswrong added a commit to piiswrong/mxnet that referenced this pull request Jan 15, 2018
@piiswrong
Revert "header updates to clarify use of CODEOWNERS (apache#9426)"
4054c80 
This reverts commit 63394de.
cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Jan 15, 2018
Revert "header updates to clarify use of CODEOWNERS (apache#9426)"
669d0bc 
This reverts commit 63394de.

This PR was merged in error
cjolivier01 pushed a commit that referenced this pull request Jan 15, 2018
@piiswrong @cjolivier01
Revert "header updates to clarify use of CODEOWNERS (#9426)" (#9442)
894c6de 
This reverts commit 63394de.
CodingCat pushed a commit to CodingCat/mxnet that referenced this pull request Jan 16, 2018
@srochel
header updates to clarify use of CODEOWNERS (apache#9426)
ee836f8 
* header updates to clarify use of CODEOWNERS

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

* fixed typo
CodingCat pushed a commit to CodingCat/mxnet that referenced this pull request Jan 16, 2018
@piiswrong
Revert "header updates to clarify use of CODEOWNERS (apache#9426)" (a…
5c35a43 
…pache#9442)

This reverts commit 63394de.
larroy pushed a commit to larroy/mxnet that referenced this pull request Jan 18, 2018
@srochel
header updates to clarify use of CODEOWNERS (apache#9426)
a34e233 
* header updates to clarify use of CODEOWNERS

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

* fixed typo
larroy pushed a commit to larroy/mxnet that referenced this pull request Jan 18, 2018
@piiswrong
Revert "header updates to clarify use of CODEOWNERS (apache#9426)" (a…
8c3ad8a 
…pache#9442)

This reverts commit 63394de.
yuxiangw pushed a commit to yuxiangw/incubator-mxnet that referenced this pull request Jan 25, 2018
@srochel @yuxiangw
header updates to clarify use of CODEOWNERS (apache#9426)
0804e76 
* header updates to clarify use of CODEOWNERS

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

* fixed typo
yuxiangw pushed a commit to yuxiangw/incubator-mxnet that referenced this pull request Jan 25, 2018
@piiswrong @yuxiangw
Revert "header updates to clarify use of CODEOWNERS (apache#9426)" (a…
b4003b8 
…pache#9442)

This reverts commit 63394de.
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@srochel @piiswrong
header updates to clarify use of CODEOWNERS (apache#9426)
bff6ff4 
* header updates to clarify use of CODEOWNERS

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

* fixed typo
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@piiswrong @cjolivier01
Revert "header updates to clarify use of CODEOWNERS (apache#9426)" (a…
53afadd 
…pache#9442)

This reverts commit 63394de.
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@srochel @zheng-da
header updates to clarify use of CODEOWNERS (apache#9426)
f7d41a2 
* header updates to clarify use of CODEOWNERS

Proposal to clarify the use of CODEOWNERS for Apache MXNet contributors and comitters.

* fixed typo
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@piiswrong @zheng-da
Revert "header updates to clarify use of CODEOWNERS (apache#9426)" (a…
5bea6ad 
…pache#9442)

This reverts commit 63394de.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjolivier01 cjolivier01 cjolivier01 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@srochel @bhavinthaker @cjolivier01 @piiswrong