This document describes the conditions and manner in which a Solr committer may commit (git commit, merge and push) new changes to non-feature branches (e.g. to master). "You" in this document is the committer wishing to make changes. These guidelines do not apply to contributors, as they do not have permissions. Feature branches are WIP that don't directly ship to a release, so they are excluded here.
Guidelines, not Rules
This document, unless stated otherwise, expresses guidelines instead of policy/rules. There is a lot of trust between committers. In some cases, it may make sense to diverge from these guidelines. We try to articulate here when that makes sense. When in doubt, ask on the dev-list.
Reviews
First and foremost, this project's official review policy is defined by the ASF as CTR (Commit-Then-Review) which grants committers permission to commit at will, with the possibility of being retroactively vetoed. However, CTR doesn't mean reviews can't come first – it just doesn't have to. In practice, code is peer-reviewed before-hand most of the time, and so this project might appear to be RTC. This project does not follow ASF's RTC (Review-Then-Commit) process which has onerous qualifications we don't wish to adhere to.
A consequence of preferring reviews first yet not strictly requiring them is the employment of an ASF concept known as lazy consensus. When this occurs, the committer gives advanced notice that he/she will commit at a defined time if there is no review. Example: "If no-one objects within two days, I'll assume lazy consensus and commit it." Do not count common holidays or weekends in those two days. Even lazy consensus is not strictly required – a committer may commit at will. See below for guidance on when up front reviews aren't worth waiting for.
What is a Review
By "Review", we mean public approval from someone other than who contributed/wrote the code. Ideally this approval is explicit like a "+1" or approval using the review system (e.g. GitHub). If the code is from a non-committer, then the reviewer is "you" here. Conversely the reviewing person might have no formal relation to the project. The approval does not have to involve a review of code when there is code. If you do such an approval, then it's helpful to indicate as-such, e.g. "+1, makes sense but I didn't look at the code". If you get feedback but no explicit approval, then use your judgment as to wether it was sufficient or declare lazy consensus in two days. Even with a clear approval, consider delaying committing if the code was quickly reviewed, thereby giving more time for feedback from others.
Admittedly we have a very low bar for what constitutes a review so as to not block progress. To make up for this, we have a high bar for becoming a committer and we put a lot of trust on the good-faith judgement of a committer to act in the project's best interest. Are you modifying code previously unfamiliar to you that you are still unsure of? Try to get a review from someone familiar with it instead of settling for less.
Don't be shy in asking others for a review. Earn goodwill and improve the project by reviewing the work of others.
Exceptions for Minor Changes
Usually we really want reviews first. But when changes are sufficiently minor (low risk, low consequence, not controversial), don't let such processes get in the way of timely progress. It needs to be minor enough that it's not worth mentioning in CHANGES.txt. You probably shouldn't bother waiting for a review for these circumstances:
- Extensibility improvements that are trivial (e.g. protected/public) on classes that are already marked with internal or experimental (or Solr classes/methods deemed similarly even if not annotated).
- Formatting / indentation
- Typo/grammar corrections
- Documentation and code comments
- Error or log message improvement
- Tests, especially additional tests
- dev-tools/
- Feature/improvement/bug-fix might all be trivial enough sometimes
Yet for any guidelines, there are counter-examples. Are you intending to auto-indent the entire codebase or do any change affecting many files? Discuss it first. Are you doing a intra-page re-organization of documentation? Try to get a review. Will your change impact backwards-compatibility? This requires a review and mention it so it's clear in the JIRA issue. Adding a new dependency (not just an updated version)? Call it out in the JIRA issue; get a review.
Please avoid making lots of consecutive little minor commits too. This is more a matter of taste in reflection of years of our practices.
JIRA has a Priority field with values Minor and Trivial, among others. We don't differentiate them here; depending on the issue, either might meet our loose definitions here to skip a review.
[PENDING DISCUSSION]: mention not needing a JIRA issue for these scenarios as well? Such a proposal needn't block a GitHub PR I suppose, and thus not all GH PRs need a JIRA issue.
Design reviews with SIP (Solr Improvement Proposals)
Once in a while we do larger architectural changes, introduce new major features/modules or change important public APIs. In such cases there may be a need for a more thorough up-front design review and a vote. Such changes typically span multiple JIRA issues and keeping the high-level design decisions a confluence page rather than in JIRA descriptions helps communicating the overall design. In addition the SIP process forces certain questions to be answered. See the main SIP page for details.
NOTE: The SIP process is still in DRAFT state and may need a vote to implement in the project policies. We may also decide to try it for 6 months and then re-evalute
Tests
[PENDING DISCUSSION]
There is no real policy on tests, but obviously, we want our code to be tested. If the proposed changes don't incorporate a test then the issue or GitHub PR (or other review platform) should mention so and why for the benefit of reviewers, unless it's obviously not needed (e.g. already tested via whatever). Trivial bugs need not.
Bug fix commits should include at least one test that does not pass without the accompanying fix.
Documentation
[PENDING DISCUSSION]
There isn't much policy on docs except for what the build enforces relating to required presence of javadocs on public classes/methods. Solr does not have that same enforcement but we view this as technical debt, at least at a class file level. For Solr, consider the impact of your change on the reference guide. Most features and improvements should update the ref guide accordingly.
Updating CHANGES.txt
[PENDING DISCUSSION]
If the change was not minor (see above RE "When Reviews Aren't Needed"), add an entry to CHANGES.txt. Choose a suitable category to add your entry generally consistent with that of the JIRA issue (although it's not 1-1 at present). Follow the formatting style you see. If there's a backwards-compatibility / upgrading concern, be sure to add an extra entry in that category.
Remember that the audience of this file serves both users and fellow committers. Consider that in use of jargon / detail, yet be brief. Readers can always read the linked JIRA issue.
Give credit to the those involved with the issue in order of decreasing importance. There is no guidance on how to deem which participants are worth a mention at all though. Sometimes committers do not even credit themselves for committing the work of a contributor. Some do but after a "via". Try to refer to people in a consistent way by searching for pre-existing reference in the file; some committers prefer their first and last name, some prefer an apache user ID.
Commit Message
[PENDING DISCUSSION]
Start a commit message with the JIRA issue ID, then a colon, space, then a one-liner brief summary. You are encouraged to add further notes on additional lines to the extent you feel is helpful. Commits frequently include various supporting changes; the commit message is a great place to list them.
If the work is primarily that of a contributor, use their name in the "Author" metadata. If you can't because you don't have their email address, then mention them in the message.
Git: Rebase vs Merge
Please set: git config --global pull.rebase true
Consequently, pushing your changes will result in your changes being moved (rebased) on top of the latest changes to the branch you push to instead of a merge commit. This keeps the history linear which is nice for simplicity sake in the vast majority of cases. However, if there are non-trivial conflicts to resolve, then abort and merge into the upstream branch instead, which generates a merge commit.