-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Proposed set of norms and best practices for issue and pull request management #78
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
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.
Thanks for putting this together.
docs/issues-pr-management.md
Outdated
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.
"teams which will work" => "teams which work"
I may be misunderstanding the purpose of the doc, but in general I think it should describe things as if we've already decided then and are operating in this fashion.
IMHO we should also minimize discussion of team boundaries.
docs/issues-pr-management.md
Outdated
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.
Will automation notify such owner(s) of such issues, or are we expected to query on a regular basis? I'm assuming I'm one of these owners, and I'm fine with either, as long as there's a good way to query for lack of any area label. Is there?
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 am not aware of such tooling but adding @karelz who may have some suggestions. In the past this problem was more simple as each repo kept on top of incoming. I would expect that in this repo the volume will be high enough that previous model will be hard to maintain.
docs/issues-pr-management.md
Outdated
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.
Should this tooling be moved to the dotnet org? Or does anyone other than Karel have rights to it?
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.
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 that is unresolved, but perhaps @jeffschwMSFT has more thoughts.
docs/issues-pr-management.md
Outdated
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.
We currently use rebase for mirroring. Does this assume we won't have an incoming mirror from mono/mono? Or this "initially" is about before mono is consolidated in?
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.
As mono/mono comes into this repo, this is an area where we will need to discuss. We are starting more restrictive, but can loosen as needs arise.
docs/issues-pr-management.md
Outdated
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'd like too see us add:
- Any branches created temporarily (e.g. as part of making a change in the GitHub web editor) must be deleted as soon as the associated PR is merged or closed.
- Any non-release/feature/master branch is subject to deletion at any time.
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.
Are we OK with such branches? I know when I've wanted to make a change in a doc, it's way easier to make it in the web editor, but then it likes to make the PR against a branch in the main fork.
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 answer my question, I think we're OK if they're created by the web editor and they're only briefly in existence.
|
@jashook looks like we need to resolve the conflict here as well because of the force-pushing. |
|
This document may be too general, but we should also have some official policy regarding the tracking of disabled tests - see #118 (comment) for the CoreCLR policy. |
This is a good point. How much does it matter if individual subfolders track disabled tests in different ways? My general litmus test is if a set of actions impacts some or all of the repo or if an action makes it feel like this repo is not one community, we should consider having central guidance. |
@jeffschwMSFT I don't think it matters much in theory, but it definitely does in practice - at least on the CoreCLR. Very often there is a rush to handle integrations and official builds that are being blocked by unreliable testing. What this means is typically someone from @dotnet/coreclr-infra would create a PR to disable the test and move on with the integration. I would assume this is something that may be frowned upon by the other code owners in the community so having a unified policy, perhaps even just for integrations/official builds, should be defined so we don't lose coverage or create conflict on testing practices. |
|
For context, this is our existing policy for CoreFx: https://github.com/dotnet/runtime/blob/master/docs/libraries/project-docs/pull-request-policy.md |
|
One more, that's the issue guide that we currently have in CoreFX: https://github.com/dotnet/runtime/blob/master/docs/libraries/project-docs/issue-guide.md. |
|
Would be great if you could make sure that we don't loose any of that information and delete those files then. |
|
Merging as our initial version. As we start to make real progress in this repo we can reopen and revisit. The outstanding discussions are around kenhub and test investigation policies. |
|
@jeffschwMSFT When an issue is opened, a bot adds the "untriaged" label. If an issue is closed with this label still attached, can the bot remove the label? Doesn't seem useful to keep the label in this case until someone manually triages it (and it seems annoying to have to query for non-closed issues). |
|
|
@jeffschwMSFT @BruceForstall the bot could be updated to remove untriaged label when an issue gets closed. |
The dotnet/runtime consolidated repository is a consolidation of the code, infrastructure, and community/team norms and practices. This document represents the collective decision making of a working group of engineers from each of the repositories that are being consolidated.