MDEV-39456: Describe the external contributions process in more details#5007
MDEV-39456: Describe the external contributions process in more details#5007
Conversation
bnestere
left a comment
There was a problem hiding this comment.
The process to address a reviewers comments might be good to include, though I don't know if we have a standard for that. I like each round of review having a single incremental commit, so it is easy to follow up and see if just the changes from that round of review are sufficient. Rather than either continually squashing everything into a single commit & having to re-review the whole patch and remember how it used to look; or a lot of very small commits together.
Thanks, I'll think about that. I guess we need to focus on the smaller contributions (90% of them). For these a single commit is just fine. Besides: github will display the changes for each subsequent commit even if it's a single commit. So I'd rather stick to the single commit situation. But it's be interesting to see what do others think. |
How? I know of the "compare" button which will |
grooverdan
left a comment
There was a problem hiding this comment.
Looking really good @gkodinov !
| * Assignee: Not relevant | ||
| * Label: not relevant | ||
| * Jira | ||
| * State: "Stalled" or "Open" |
| * contain a concise description of what the change is: this goes to the git commit log and is used for reference. | ||
| * should contain all the relevant attribution headers (co-authors, reviewers etc.) | ||
| * The pull request should contain at least a copy of the commit message. Ideally more can be added to it. | ||
| * The pull request should target the right branch. Rule of thumb: the lowest affected activelly supported version for bugs, main branch for features and refactorings. |
There was a problem hiding this comment.
actively
Link to https://mariadb.org/about/#maintenance-policy.
The fixVersion on a JIRA issue can be used as a guide, but limit to actively supported versions,but not rolling releases about to go EOL.
| * should contain all the relevant attribution headers (co-authors, reviewers etc.) | ||
| * The pull request should contain at least a copy of the commit message. Ideally more can be added to it. | ||
| * The pull request should target the right branch. Rule of thumb: the lowest affected activelly supported version for bugs, main branch for features and refactorings. | ||
| * All the relevant buildbot tests should be passing (or, if failing, the failures should be studied and justified as unrelated) |
There was a problem hiding this comment.
Maybe more detail:
Compare to the tests that succeed on the target branch ref: https://buildbot.mariadb.org/#/grid?branch=BRANCHNAME
Check test output if assessing that a test failure is identical.
| * All the relevant buildbot tests should be passing (or, if failing, the failures should be studied and justified as unrelated) | ||
| * The licensing of the pull request should be clear. Ideally the CLA buildbot should be green. If you can't do that mentioning a license in the pull request's description can be a workaround. | ||
|
|
||
| Questions and Answers |
There was a problem hiding this comment.
Move this to the end under its own heading?
- Who creates a Jira issue?es
- anyone can create a JIRA issue
- Can I get a design review of a Jira issue in advance of writing a PR?
- Yes, create a zulip topic and ask if no response within a few days.
- Does fixing a typo, documentation or code comments really require a Jira issue?
- If the issue/change isn't going to make it to a release notes, then a Jira isn't needed.
There was a problem hiding this comment.
I've added the first question. I think the 2nd drifts away a bit. There's a way to contact us in CONTRIBUTING.md already.
And I don't agree with the 3d question's answer. It was made very clear to me by the management that everything should get a jira.
| @@ -0,0 +1,196 @@ | |||
| # Community Contributions Process # | |||
|
|
|||
There was a problem hiding this comment.
I think a statement of how MariaDB values community contributions would be a good way to begin this document.
Every PR update has "compare" button, which gives exactly what you describe. It is also relatively easy to get this effect locally. It can be spoiled by rebase though. OTOH incremental commits in early review stages are probably alright. When it comes closer to merge they need to be squashed anyway. |
|
|
||
| The pull request is used to describe how the bug is fixed. Or how the feature is implemented in detail. | ||
|
|
||
| A pull request should contain a **signle commit**! And that commit should have a commit message that's compliant with the MariaDB coding standards. |
There was a problem hiding this comment.
We don't follow this rule ourselves. It's be odd if I were forced to use single commit e.g. in #4048.
Commit has to be self-contained isolated change, while PR spans such changes to form a solution.
I can imagine the intention was avoiding incremental commits, probably also unjustified commit splitting. In this case it should be stated clearly and distinguished from justified multi-commit PRs.
There was a problem hiding this comment.
well, that's the intent at least. There's always exceptions.
ea460ce to
7b60769
Compare
I believe they're quite readable. E.g. take #5011. The submitter has pushed an initial commit. Then they've updated it with --force. And the Compare link is pretty readable IMHO. |
|
|
||
| ### What should go into the jira ### | ||
|
|
||
| The [Jira](https://jira.mariadb.org) is used to describe the design of the |
There was a problem hiding this comment.
Contributors can save themselves time reworking code if they include implementation details for the bug or feature before they start coding. This provides an opportunity for early feedback that might impact the code solution.
There was a problem hiding this comment.
Added:
Tip
A lot of time an energy can be saved, especially on more complex tasks,
by communicating early with possible reviewers and writing down a design
specification into the Jira prior to implementation
| * Jira | ||
| * State: "Open", "In Progress", "Stalled" or "Needs feedback" | ||
| * Assignee: The preliminary reviewer | ||
| * Label: "Contribution" |
There was a problem hiding this comment.
Why do we have "Contribution" and "External Contribution" labels depending on the review stage? Isn't an external contribution an external contribution through all stages?
There was a problem hiding this comment.
It's the pull requests having the "External contributions" label. And Jiras having "contribution". These were pre-existing. Just quoting them here.
If a contributor only |
Added a new .md document describing the community contribution process. Added a reference to it from the CONTRIBUTING.md.
7b60769 to
bafe1a5
Compare
You can't win them all. We do not have to account for all the github bugs. |
Added a new COMMUNITY_CONTRIBUTIONS.md document describing the community contribution process.
Added a reference to it from the CONTRIBUTING.md.