-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Remove CODEOWNERS #1367
Remove CODEOWNERS #1367
Conversation
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.
LG with some suggestions. Holding off on approving just to make sure no one else wants a look...
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
We suggest a specific workflow to address this (note, commit access is | ||
required): |
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.
Do we still want to suggest this? I've not really seen people using it recently, and my experience trying it previously was not all that pleasant -- to the point that I find I prefer to delay reviews of dependent changes until the base change lands. It also seems exclusionary to provide a different workflow for people with commit access versus everyone else, especially if we do not intend for commit access to be granted to all regular 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.
Personally, I'd support removing it, but IIRC @chandlerc added it so it may be best to discuss with him.
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.
Nah, remove it. It just doesn't work. I've switched to just documenting in stacked PRs which commit range to look at and giving up once merges are occuring.
…ow/carbon-lang into proposal-remove-codeowners
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
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.
LG to me. No need to block on the question of what we do with dependent PRs, I think we can work that out separately, but we should open an issue with the question.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
TODO: Add doc for who owns what (docs/project/owners.md?) |
Co-authored-by: Geoff Romer <gromer@google.com>
…ow/carbon-lang into proposal-remove-codeowners
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.
LGTM, thanks for the added detail.
Versus #1367 which approved this in principle, this is the actual removal (I've updated write permissions in GH now).
Wiki can either be "require push access" or "everyone" -- apparently there's no other option. I've set it to "everyone" so that contributors can make edits without needing push access. So the options as I see it are: - Leave wiki as "everyone" can edit, use this for notifications. - GitHub doesn't give notifications for wiki edits. This is trying a different approach for notifications. - Grant push access to a larger group (contributors), don't add CODEOWNERS. - Not sure this is the right choice because of the implications around merges, but again maybe it'd be fine and we can expect the approval requirement to work out. - Grant push access to contributors, add CODEOWNERS. - This causes the auto-assignment to CODEOWNERS that we don't want. (details in #1367) - Create a separate wiki repo so that we can differently handle push access. - This seems overly complex a solution though. I'm hoping this approach works.
Remove CODEOWNERS and rely on repository commit access
DO NOT MERGE