Issue 6817113: Add CONTRIBUTING (Closed)
DescriptionAdd CONTRIBUTING
https://code.launchpad.net/~dave-cheney/juju-core/045-CONTRIBUTORS/+merge/133852
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 22
Patch Set 2 : Add CONTRIBUTING #
Total comments: 14
Patch Set 3 : Add CONTRIBUTING #
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
LGTM, just a few suggestions. (would also be kinda nice to just fix the TODOs for this first commit -- they don't look like they're a great deal of work (?)) https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode4 CONTRIBUTING:4: Before contributing to juju-core please read the following sections describing the tools and conventions of this project. This file is a companion to README and it is assumed that file has been read prior. I'd really like it if this were all wrapped to ~80 chars. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode9 CONTRIBUTING:9: juju-core uses bzr for source control. To make the bzr branching strategy more palatable for the go tool set juju-core recommends using the cobzr helper. cobzr allows the user to juggle multiple branches in place in a way that is compatible with GOPATH. While it is possible to manage bzr branches manually, the remainder of this document will assume cobzr is in use as practiced by the juju-core team. s/tool set/toolset/ https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode86 CONTRIBUTING:86: lbox will prompt you for a branch description and by default will create a rietveld code review for this change, linking it to the launchpad branch, and mailing reviewers. All code review is done on reitveld, not launchpad. The lbox tool manages the state of the launchpad review, with the exception of marking reviews as rejected or work in progress by the reviewer. If you abandon a proposal, you should mark it as rejected in launchpad to avoid others wasting time reviewing it. If you decide to start again, you should add a comment to the abandoned reitveld proposal linking it to your replacement. s/reitveld/rietveld/ https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode108 CONTRIBUTING:108: Note, there is currently a bug with lbox when linking a branch to a bug that sets the milestone field to the last milestone defined on the project. Generally this is not what you want, so you should visit the bug's page and correct the milestone, if set. I'd almost prefer not to mention -bug at all (doesn't it still trash the bug's description too? or maybe that was fixed, I forget...)
Sign in to reply to this message.
couple more comments https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode37 CONTRIBUTING:37: All changes to juju-core must be performed on a branch, that branch reviewed, then submitted. An overview of the commands used to do so follows. These examples use the bzr command, which is assumed to be aliased to cobzr, as described above. First, create a branch for your change. In IRC, sidnei noted that this is a little bit unclear: I *think* that should be fixable by just reiterating that you're expected to run these commands in the go got juju-core directory. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode43 CONTRIBUTING:43: bzr switch add-CONTRIBUTING Or you could just use `bzr checkout -b`, as sidnei points out.
Sign in to reply to this message.
Awesome, thanks Dave. LGTM https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode4 CONTRIBUTING:4: Before contributing to juju-core please read the following sections describing the tools and conventions of this project. This file is a companion to README and it is assumed that file has been read prior. On 2012/11/12 07:25:21, fwereade wrote: > I'd really like it if this were all wrapped to ~80 chars. +1! https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode66 CONTRIBUTING:66: extends the previous example by enabling the juju-core/log package's debug flag. By default gocheck will run all tests in a package, selected tests can by run by passing -gocheck.f That's not actually the case. Logs are always on, even without .v or .vv. What .v does is to expose the tests executed even if they pass, and .vv will run in unbuffered mode, and show everything logged as the test runs, even for successes. Logs are always shown for .v and without either of them on failures. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode82 CONTRIBUTING:82: Once you change is ready, and you have run all the tests successfully you can propose your branch for merging s/you change/your change/ https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode86 CONTRIBUTING:86: lbox will prompt you for a branch description and by default will create a rietveld code review for this change, linking it to the launchpad branch, and mailing reviewers. All code review is done on reitveld, not launchpad. The lbox tool manages the state of the launchpad review, with the exception of marking reviews as rejected or work in progress by the reviewer. If you abandon a proposal, you should mark it as rejected in launchpad to avoid others wasting time reviewing it. If you decide to start again, you should add a comment to the abandoned reitveld proposal linking it to your replacement. On 2012/11/12 07:25:21, fwereade wrote: > s/reitveld/rietveld/ There are occurrences in the rest of the doc too. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode108 CONTRIBUTING:108: Note, there is currently a bug with lbox when linking a branch to a bug that sets the milestone field to the last milestone defined on the project. Generally this is not what you want, so you should visit the bug's page and correct the milestone, if set. On 2012/11/12 07:25:21, fwereade wrote: > I'd almost prefer not to mention -bug at all (doesn't it still trash the bug's > description too? or maybe that was fixed, I forget...) I've never seen that happening. If the bug exists, it should just attach it to the proposal. I think it's fine to mention it, and if there are bugs please let me know. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode120 CONTRIBUTING:120: Before proposing, lbox runs a number of hooks to improve code quality and ensure that code is properly formatted. These checks are in $GOPATH/launchpad.net/juju-core/.lbox.check. They are run automatically by lbox before proposing or submitting. If they hooks fail you will have to resolve those errors before trying again. For example s/If they hooks fail you/If these hooks fail, you/
Sign in to reply to this message.
Thank you for your comments. I have also made the formatting look moar like markdown. YMMV https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode4 CONTRIBUTING:4: Before contributing to juju-core please read the following sections describing the tools and conventions of this project. This file is a companion to README and it is assumed that file has been read prior. On 2012/11/12 07:25:21, fwereade wrote: > I'd really like it if this were all wrapped to ~80 chars. Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode9 CONTRIBUTING:9: juju-core uses bzr for source control. To make the bzr branching strategy more palatable for the go tool set juju-core recommends using the cobzr helper. cobzr allows the user to juggle multiple branches in place in a way that is compatible with GOPATH. While it is possible to manage bzr branches manually, the remainder of this document will assume cobzr is in use as practiced by the juju-core team. On 2012/11/12 07:25:21, fwereade wrote: > s/tool set/toolset/ Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode37 CONTRIBUTING:37: All changes to juju-core must be performed on a branch, that branch reviewed, then submitted. An overview of the commands used to do so follows. These examples use the bzr command, which is assumed to be aliased to cobzr, as described above. First, create a branch for your change. On 2012/11/12 13:41:52, fwereade wrote: > In IRC, sidnei noted that this is a little bit unclear: I *think* that should be > fixable by just reiterating that you're expected to run these commands in the go > got juju-core directory. Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode43 CONTRIBUTING:43: bzr switch add-CONTRIBUTING On 2012/11/12 13:41:52, fwereade wrote: > Or you could just use `bzr checkout -b`, as sidnei points out. Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode66 CONTRIBUTING:66: extends the previous example by enabling the juju-core/log package's debug flag. By default gocheck will run all tests in a package, selected tests can by run by passing -gocheck.f On 2012/11/12 13:53:04, niemeyer wrote: > That's not actually the case. Logs are always on, even without .v or .vv. What > .v does is to expose the tests executed even if they pass, and .vv will run in > unbuffered mode, and show everything logged as the test runs, even for > successes. Logs are always shown for .v and without either of them on failures. Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode82 CONTRIBUTING:82: Once you change is ready, and you have run all the tests successfully you can propose your branch for merging On 2012/11/12 13:53:04, niemeyer wrote: > s/you change/your change/ Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode86 CONTRIBUTING:86: lbox will prompt you for a branch description and by default will create a rietveld code review for this change, linking it to the launchpad branch, and mailing reviewers. All code review is done on reitveld, not launchpad. The lbox tool manages the state of the launchpad review, with the exception of marking reviews as rejected or work in progress by the reviewer. If you abandon a proposal, you should mark it as rejected in launchpad to avoid others wasting time reviewing it. If you decide to start again, you should add a comment to the abandoned reitveld proposal linking it to your replacement. On 2012/11/12 07:25:21, fwereade wrote: > s/reitveld/rietveld/ Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode86 CONTRIBUTING:86: lbox will prompt you for a branch description and by default will create a rietveld code review for this change, linking it to the launchpad branch, and mailing reviewers. All code review is done on reitveld, not launchpad. The lbox tool manages the state of the launchpad review, with the exception of marking reviews as rejected or work in progress by the reviewer. If you abandon a proposal, you should mark it as rejected in launchpad to avoid others wasting time reviewing it. If you decide to start again, you should add a comment to the abandoned reitveld proposal linking it to your replacement. On 2012/11/12 13:53:04, niemeyer wrote: > On 2012/11/12 07:25:21, fwereade wrote: > > s/reitveld/rietveld/ > > There are occurrences in the rest of the doc too. Done. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode108 CONTRIBUTING:108: Note, there is currently a bug with lbox when linking a branch to a bug that sets the milestone field to the last milestone defined on the project. Generally this is not what you want, so you should visit the bug's page and correct the milestone, if set. On 2012/11/12 07:25:21, fwereade wrote: > I'd almost prefer not to mention -bug at all (doesn't it still trash the bug's > description too? or maybe that was fixed, I forget...) It doens't break the desc anymore, just fudge the milestone. https://codereview.appspot.com/6817113/diff/1/CONTRIBUTING#newcode108 CONTRIBUTING:108: Note, there is currently a bug with lbox when linking a branch to a bug that sets the milestone field to the last milestone defined on the project. Generally this is not what you want, so you should visit the bug's page and correct the milestone, if set. On 2012/11/12 13:53:04, niemeyer wrote: > On 2012/11/12 07:25:21, fwereade wrote: > > I'd almost prefer not to mention -bug at all (doesn't it still trash the bug's > > description too? or maybe that was fixed, I forget...) > > I've never seen that happening. If the bug exists, it should just attach it to I > the proposal. I think it's fine to mention it, and if there are bugs please let > me know. I believe there is a bug open for this, i will check
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode52 CONTRIBUTING:52: First, create a branch for your change. s/$/using the following command/ https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode56 CONTRIBUTING:56: Will branch `juju-core` and create a new branch called `add-CONTRIBUTING` in your s/^/This / https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode67 CONTRIBUTING:67: to match the state of the `add-CONTRIBUTING` branch. It is a good idea to make s/It is a good idea/You must/ https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode124 CONTRIBUTING:124: `lbox` will prompt you for a branch description and by default will create a s/by default// https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode124 CONTRIBUTING:124: `lbox` will prompt you for a branch description and by default will create a There should be a mention that the first time lbox runs, it opens a browser window and requires some access approval in launchpad. This confused the hell out of me (I ran it under remote X). https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode125 CONTRIBUTING:125: rietveld code review for this change, linking it to the launchpad branch, and What is rietveld? I propose mentioning rietveld in the section synopsis and eliding here. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode155 CONTRIBUTING:155: Finally, if your proposal fixes a bug, you can use the `-bug` flag to link the By default, lbox creates a new bug in launchpad for each proposed branch, and assigns the branch to it. If you wish to assign it to an existing bug instead, use... https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode160 CONTRIBUTING:160: Note, there is currently a bug with `lbox` when linking a branch to a bug which s/Note, /
Sign in to reply to this message.
I forgot to LGTM.
Sign in to reply to this message.
*** Submitted: Add CONTRIBUTING R=fwereade, niemeyer, aram CC= https://codereview.appspot.com/6817113 https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING File CONTRIBUTING (right): https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode52 CONTRIBUTING:52: First, create a branch for your change. On 2012/11/13 10:31:55, aram wrote: > s/$/using the following command/ Done. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode56 CONTRIBUTING:56: Will branch `juju-core` and create a new branch called `add-CONTRIBUTING` in your On 2012/11/13 10:31:55, aram wrote: > s/^/This / Done. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode67 CONTRIBUTING:67: to match the state of the `add-CONTRIBUTING` branch. It is a good idea to make On 2012/11/13 10:31:55, aram wrote: > s/It is a good idea/You must/ Done. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode124 CONTRIBUTING:124: `lbox` will prompt you for a branch description and by default will create a On 2012/11/13 10:31:55, aram wrote: > There should be a mention that the first time lbox runs, it opens a browser > window and requires some access approval in launchpad. This confused the hell > out of me (I ran it under remote X). Done. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode125 CONTRIBUTING:125: rietveld code review for this change, linking it to the launchpad branch, and On 2012/11/13 10:31:55, aram wrote: > What is rietveld? > > I propose mentioning rietveld in the section synopsis and eliding here. I was lazy and just added a hyperlink. https://codereview.appspot.com/6817113/diff/5002/CONTRIBUTING#newcode160 CONTRIBUTING:160: Note, there is currently a bug with `lbox` when linking a branch to a bug which On 2012/11/13 10:31:55, aram wrote: > s/Note, / Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
