Guidelines for Committing Code to DSpace GitHub

General Guidelines for Committing

1. Use JIRA to track your changes
   • Ensure there is an issue describing the fix/improvement/feature in JIRA (If this is a minor typo fix, or an obvious bug, you need not add a JIRA issue – use your judgment on whether this fix is worth tracking in JIRA or not)
   • Please note that our Documentation History page is generated automatically from JIRA. Therefore any change that should appear on that page must be added as a JIRA ticket.
2. Submit a Pull Request for your code changes
   • First off, you should be doing the majority of your development in your own GitHub Fork. See Development with Git.
   • Generally speaking, code changes should come in via a Pull Request. This gives the rest of the Committers/Developers a chance to comment on the Pull Request.
   • If this is a very minor fix/change (typo / improved comments / obvious bug), you can bypass creating a Pull Request and commit it immediately. This is an exception to the rule.
3. Code approval processes
   • General Rule of Thumb: Committers are trusted developers. Use your best judgment on whether or not you feel a particular piece of code should be approved before committing/merging your Pull Request. The below guidelines are just suggestions and they may not apply to all scenarios:
   • If this is a small, obvious fix (typo / improved comments / obvious bug), just commit it immediately. This is the exception to all other rules – there's no reason to add a JIRA issue for every typo or obvious fix, and no reason to wait around for approval to commit it.
   • If this is a bug/issue fix, try to get approval for your Pull Request from at least one other Committer. If there is any doubt, feel free to run it by others or ask to have it added to the agenda of the next developers meeting.
   • If this is a new feature, generally speaking you need at least three Committers to approve (or bring it up in a developer meeting, if it's a larger change). The only reason we suggest to run it by more than one committer is that oftentimes others may be able to provide additional suggestions or configurations that may be worth investigating.
   • Please note that our Developer Voting Procedures actually state that at least three Committers should approve of all code changes, and none should disapprove. We tend to be lenient on minor code changes / bug fixes (as these tend to be non-controversial changes). But, new features should really get the approval of at least three Committers (and larger features should be discussed / voted on in a public forum, e.g. IRC meeting or email).
4. Document your code early & often
   • New features/improvements need to come with documentation, otherwise we won't be able to accept them. Keep this in mind while you are developing, and use the Wiki or JIRA to start your documentation early on (it'll make it easier on you in the long run).
5. Once Approved, Merge your Pull Request in a timely manner
   • Once your Pull Request is approved, you should work to merge it as soon as you can. This will avoid your Pull Request getting "stale" and ensure that any upcoming work can take it into account.
   • Before merging, please ensure that your code doesn't break any Unit Tests. This will save you a potential headache later, when our Continuous Integration system does a verification of all Unit Tests.
   • If the Pull Request was not created by a Committer, the Committer(s) who are most familiar with the work (and who may have been supporting/giving feedback) should merge the Pull Request.
   • Obviously, Committers can ask for someone else to take over the merge process as needed.

GitHub "Master Branch" Committing Rules

The GitHub Master branch is one of the few places that needs to remain well managed. As we all fork it and/or create Pull Requests from it, it can be detrimental to us all if incomplete or extremely buggy code is committed. Obviously, mistakes happen (and we've all made them), so don't worry if you accidentally commit/merge something you didn't mean to (just try to roll it back as soon as you notice it). Here are the few rules we try to follow when committing code to the Master branch:

1. No broken builds on the Master Branch, ever.
   • All commits / Pull Requests should be tested beforehand to ensure they will not "break" the Master Branch (this includes Unit Testing the code to ensure our Continuous Integration system doesn't report a unit test failure).
   • A broken build is a matter of urgency. Mistakes happen, but if you break the build, please make sure to fix it ASAP or temporarily rollback your changes until you can resolve the issues.
   • Obviously, there may be situations where temporarily breaking the build may be necessary. However, we should make all attempts to keep breakage to a minimum (or forewarn the Committer team if anticipated). Longer-term breakage (a few days or more) can negatively affect us all.
2. No unapproved / incomplete features on the Master Branch.
   • As mentioned, all your code / new feature development should take place in your own fork and submitted via a Pull Request once it is ready for feedback (see Development with Git). The only exception to this rule is trivial changes (e.g. typos / improved comments / obvious bug fixes).
   • Your work should have as close to a "complete feature set" as possible. Note that this doesn't necessarily mean all work is "completed", but the code should not make the system difficult to use in the near term or otherwise hamper other development efforts.
   • If your code is known to be "incomplete" in any way, you must make this clear to the Committers and detail what work is still required.
   • Before your work can be merged, it must satisfy Rule #1 AND be approved for merging by the Committers (See "code approval processes" note in the #General Guidelines for Committing above). During the "release window", the Release Team has the final say.
   • Once something is merged to master, we are effectively committed to it being in the next release.
3. All new features must have documentation before merging into the Master Branch.
   • New features/improvements should not be merged into the Master Branch until there is some minimal documentation (minimal documentation includes documenting all configuration options). Ideally, they should also come with usage documentation (i.e. examples of how to use the feature, how to configure the feature, etc.) This will help us all ensure that Documentation is ready by the time we get to the next release, and hopefully lessen the time that any one person has to spend cleaning up or rewriting documentation.