1d0c7e27eSEmily Shaffer# Contributing Guidelines 2d0c7e27eSEmily Shaffer 3d0c7e27eSEmily ShafferThis document attempts to outline some basic rules to follow when contributing 4*a1bd285eSPatrick Williamsto OpenBMC's IPMI stack. It does _not_ outline coding style; we follow the 5d0c7e27eSEmily Shaffer[OpenBMC C++ style guide](https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions) 6d0c7e27eSEmily Shafferfor that. 7d0c7e27eSEmily Shaffer 8d0c7e27eSEmily Shaffer## Organizing Commits 9d0c7e27eSEmily Shaffer 10d0c7e27eSEmily ShafferA good commit does exactly one thing. We prefer many small, atomic commits to 11d0c7e27eSEmily Shafferone large commit which makes many functional changes. 12*a1bd285eSPatrick Williams 13d0c7e27eSEmily Shaffer- Too large: "convert foo to new API; also fix CVE 1234 in bar" 14d0c7e27eSEmily Shaffer- Too small: "move abc.h to top of include list" and "move xyz.h to bottom of 15d0c7e27eSEmily Shaffer include list" 16d0c7e27eSEmily Shaffer- Just right: "convert foo to new API" and "convert foo from tab to space" 17d0c7e27eSEmily Shaffer 18d0c7e27eSEmily ShafferOften, creating small commits this way results in a number of commits which are 19d0c7e27eSEmily Shafferdependent on prior commits; Gerrit handles this situation well, so feel free to 20d0c7e27eSEmily Shafferpush commits which are based on your change still in review. However, when 21d0c7e27eSEmily Shafferpossible, your commit should stand alone on top of master - "Fix whitespace in 22d0c7e27eSEmily Shafferbar()" does not need to depend on "refactor foo()". Said differently, ensure 23d0c7e27eSEmily Shafferthat topics which are not related to each other semantically are also not 24d0c7e27eSEmily Shafferrelated to each other in Git until they are merged into master. 25d0c7e27eSEmily Shaffer 26d0c7e27eSEmily ShafferWhen pushing a stack of patches (current branch is >1 commits ahead of 27d0c7e27eSEmily Shafferorigin/master), these commits will show up with that same relationship in 28d0c7e27eSEmily Shaffergerrit. This means that each patch must be merged in order of that relationship. 29d0c7e27eSEmily ShafferSo if one of the patches in the middle needs to be changed, all the patches from 30d0c7e27eSEmily Shafferthat point on would need to be pushed to maintain the relationship. This will 31d0c7e27eSEmily Shaffereffectively rebase the unchanged patches, which would in turn trigger a new CI 32d0c7e27eSEmily Shafferbuild. Ideally, changes from the entire patchset could be done all in one go to 33d0c7e27eSEmily Shafferreduce unnecessary rebasing. 34d0c7e27eSEmily Shaffer 35d0c7e27eSEmily ShafferWhen someone makes a comment on your commit in Gerrit, modify that commit and 36d0c7e27eSEmily Shaffersend it again to Gerrit. This typically involves `git rebase --interactive` or 37d0c7e27eSEmily Shaffer`git commit --amend`, for which there are many guides online. As mentioned in 38d0c7e27eSEmily Shafferthe paragraph above, when possible you should make changes to multiple patches 39d0c7e27eSEmily Shafferin the same stack before you push, in order to minimize CI and notification 40d0c7e27eSEmily Shafferchurn from the rebase operations. 41d0c7e27eSEmily Shaffer 42d0c7e27eSEmily ShafferCommits which include changes that can be tested by a unit test should also 43d0c7e27eSEmily Shafferinclude a unit test to exercise that change, within the same commit. Unit tests 44d0c7e27eSEmily Shaffershould be clearly written - even moreso than production code, unit tests are 45d0c7e27eSEmily Shaffermeant primarily to be read by humans - and should test both good and bad 46d0c7e27eSEmily Shafferbehaviors. Refer to the 47d0c7e27eSEmily Shaffer[testing documentation](https://github.com/openbmc/phosphor-host-ipmid/blob/master/docs/testing.md) 48d0c7e27eSEmily Shafferfor help writing tests, as well as best practices. 49d0c7e27eSEmily Shaffer 50d0c7e27eSEmily Shaffer## Formatting Commit Messages 51d0c7e27eSEmily Shaffer 52d0c7e27eSEmily ShafferYour commit message should explain: 53d0c7e27eSEmily Shaffer 54*a1bd285eSPatrick Williams- Concisely, _what_ change are you making? e.g. "docs: convert from US to UK 55d0c7e27eSEmily Shaffer spelling" This first line of your commit message is the subject line. 56*a1bd285eSPatrick Williams- Comprehensively, _why_ are you making that change? In some cases, like a small 57*a1bd285eSPatrick Williams refactor, the why is fairly obvious. But in cases like the inclusion of a new 58*a1bd285eSPatrick Williams feature, you should explain why the feature is needed. 59*a1bd285eSPatrick Williams- Concisely, _how_ did you test? This comes in the form of a "Tested:" footer in 60*a1bd285eSPatrick Williams your commit message and is required for all code changes in the IPMI stack. It 61*a1bd285eSPatrick Williams typically consists of copy-pasted ipmitool requests and responses. When 62*a1bd285eSPatrick Williams possible, use the high-level ipmitool commands (e.g. "ipmitool sensor read 63*a1bd285eSPatrick Williams 0x1"). In cases where that's not possible, or when testing edge or error 64d0c7e27eSEmily Shaffer cases, it is acceptable to use "ipmitool raw" - but an explanation of your 65*a1bd285eSPatrick Williams output is appreciated. If the change can be validated entirely by running unit 66*a1bd285eSPatrick Williams tests, say so in the "Tested:" tag. 67d0c7e27eSEmily Shaffer 68d0c7e27eSEmily ShafferTry to include the component you are changing at the front of your subject line; 69d0c7e27eSEmily Shafferthis typically comes in the form of the class, module, handler, or directory you 70d0c7e27eSEmily Shafferare modifying. e.g. "apphandler: refactor foo to new API" 71d0c7e27eSEmily Shaffer 72d0c7e27eSEmily ShafferLoosely, we try to follow the 50/72 rule for commit messages - that is, the 73d0c7e27eSEmily Shaffersubject line should not exceed 50 characters and the body should not exceed 72 74d0c7e27eSEmily Shaffercharacters. This is common practice in many projects which use Git. 75d0c7e27eSEmily Shaffer 76d0c7e27eSEmily ShafferAll commit messages must include a Signed-off-by line, which indicates that you 77d0c7e27eSEmily Shafferthe contributor have agreed to the Developer Certificate of Origin. This line 78d0c7e27eSEmily Shaffermust include the name you commonly use, often a given name and a family name or 79*a1bd285eSPatrick Williamssurname. (ok: A. U. Thor, Sam Samuelsson, robert a. heinlein; not ok: xXthorXx, 80*a1bd285eSPatrick WilliamsSam, RAH) 81d0c7e27eSEmily Shaffer 82d0c7e27eSEmily Shaffer## Sending Patches 83d0c7e27eSEmily Shaffer 84*a1bd285eSPatrick WilliamsLike most projects in OpenBMC, we use Gerrit to review patches. Please check the 85*a1bd285eSPatrick WilliamsMAINTAINERS file to determine who needs to approve your review in order for your 86*a1bd285eSPatrick Williamschange to be merged. Submitters will need to manually add their reviewers in 87*a1bd285eSPatrick WilliamsGerrit; reviewers are not currently added automatically. Maintainers may not see 88*a1bd285eSPatrick Williamsthe commit if they have not been added to the review! 89d0c7e27eSEmily Shaffer 90d0c7e27eSEmily Shaffer## Pace of Review 91d0c7e27eSEmily Shaffer 92d0c7e27eSEmily ShafferContributors who are used to code reviews by their team internal to their own 93d0c7e27eSEmily Shaffercompany, or who are not used to code reviews at all, are sometimes surprised by 94d0c7e27eSEmily Shafferthe pace of code reviews in open source projects. Try to keep in mind that those 95d0c7e27eSEmily Shafferreviewing your patch may be contributing to OpenBMC in a volunteer or 96d0c7e27eSEmily Shafferpartial-time capacity, may be in a timezone far removed from your own, and may 97d0c7e27eSEmily Shafferhave very deep review queues already of patches which have been waiting longer 98d0c7e27eSEmily Shafferthan yours. 99d0c7e27eSEmily Shaffer 100*a1bd285eSPatrick WilliamsIf you feel your patch has been missed entirely, of course it's alright to email 101*a1bd285eSPatrick Williamsthe maintainers (addresses available in MAINTAINERS file) - but a reasonable 102*a1bd285eSPatrick Williamstimeframe to do so is on the order of a week, not on the order of hours. 103d0c7e27eSEmily Shaffer 104d0c7e27eSEmily ShafferAdditionally, the IPMI stack has a set of policies for when and how changes can 105d0c7e27eSEmily Shafferbe approved; please check the MAINTAINERS file for the full list ("Change 106d0c7e27eSEmily Shafferapproval rules"). 107d0c7e27eSEmily Shaffer 108d0c7e27eSEmily ShafferThe maintainers' job is to ensure that incoming patches are as correct and easy 109d0c7e27eSEmily Shafferto maintain as possible. Part of the nature of open source is attrition - 110d0c7e27eSEmily Shaffercontributors can come and go easily - so maintainers tend not to put stock in 111d0c7e27eSEmily Shafferpromises such as "I will add unit tests in a later patch" or "I will be 112d0c7e27eSEmily Shafferimplementing this proposal by the end of next month." This often manifests as 113d0c7e27eSEmily Shafferreviews which may seem harsh or exacting; please keep in mind that the community 114d0c7e27eSEmily Shafferis trying to collaborate with you to build a patch that will benefit the project 115d0c7e27eSEmily Shafferon its own. 116d0c7e27eSEmily Shaffer 117d0c7e27eSEmily Shaffer## Code of Conduct 118d0c7e27eSEmily Shaffer 119d0c7e27eSEmily ShafferWe enthusiastically adhere to the same 120d0c7e27eSEmily Shaffer[Code of Conduct](https://github.com/openbmc/docs/blob/master/code-of-conduct.md) 121d0c7e27eSEmily Shafferas the rest of OpenBMC. If you have any concerns, please check that document for 122d0c7e27eSEmily Shafferguidelines on who can help you resolve them. 123