713.3.1
by Dave Cheney
added CONTRIBUTING |
1 |
Getting started |
2 |
===============
|
|
3 |
||
713.3.4
by Dave Cheney
responding to review feedback |
4 |
Before contributing to `juju-core` please read the following sections describing |
5 |
the tools and conventions of this project. This file is a companion to README |
|
6 |
and it is assumed that file has been read prior. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
7 |
|
8 |
cobzr
|
|
9 |
-----
|
|
10 |
||
713.3.4
by Dave Cheney
responding to review feedback |
11 |
juju-core uses bzr for source control. To make the bzr branching strategy more |
12 |
palatable for the go toolset `juju-core` recommends using the `cobzr` helper. |
|
13 |
`cobzr` allows the user to juggle multiple branches in place in a way that is |
|
14 |
compatible with GOPATH. While it is possible to manage bzr branches manually, |
|
15 |
the remainder of this document will assume cobzr is in use as practiced by the |
|
16 |
juju-core team. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
17 |
|
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
18 |
To install `cobzr`, |
713.3.1
by Dave Cheney
added CONTRIBUTING |
19 |
|
20 |
go get launchpad.net/cobzr |
|
21 |
||
22 |
TODO(dfc) document PPA version |
|
23 |
||
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
24 |
It is recommended to use alias `cobzr` to `bzr`. |
713.3.1
by Dave Cheney
added CONTRIBUTING |
25 |
|
26 |
alias bzr=cobzr |
|
27 |
||
713.3.4
by Dave Cheney
responding to review feedback |
28 |
As `cobzr` passes any options it does not handle to `bzr`, it is safe, and recommended |
29 |
to add this alias command to your login shell. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
30 |
|
31 |
lbox
|
|
32 |
----
|
|
33 |
||
713.3.4
by Dave Cheney
responding to review feedback |
34 |
`juju-core` uses `lbox` for code review and submission. In addition `lbox` enables |
35 |
the use of prerequisite branches. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
36 |
|
37 |
To install lbox |
|
38 |
||
39 |
go get launchpad.net/lbox |
|
40 |
||
41 |
TODO(dfc) document PPA version |
|
42 |
||
43 |
Branching
|
|
44 |
=========
|
|
45 |
||
713.3.4
by Dave Cheney
responding to review feedback |
46 |
All changes to `juju-core` must be performed on a branch, that branch reviewed, |
47 |
then submitted. An overview of the commands used to do so follows. These |
|
48 |
examples use the `bzr` command, which is assumed to be aliased to `cobzr`, as |
|
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
49 |
described above. It is also assumed that your working directory is |
826.1.1
by Dave Cheney
fix incorrect src path |
50 |
$GOPATH/src/launchpad.net/juju-core. |
713.3.4
by Dave Cheney
responding to review feedback |
51 |
|
713.3.5
by Dave Cheney
final tweaks |
52 |
First, create a branch for your change using the following command |
713.3.1
by Dave Cheney
added CONTRIBUTING |
53 |
|
54 |
bzr branch lp:juju-core/ add-CONTRIBUTING |
|
55 |
||
713.3.5
by Dave Cheney
final tweaks |
56 |
This will branch `juju-core` and create a new branch called `add-CONTRIBUTING` in |
57 |
your local workspace. Importantly this will not switch to this branch immediately, |
|
713.3.4
by Dave Cheney
responding to review feedback |
58 |
so to switch to this branch use the following |
713.3.1
by Dave Cheney
added CONTRIBUTING |
59 |
|
60 |
bzr switch add-CONTRIBUTING |
|
61 |
||
713.3.4
by Dave Cheney
responding to review feedback |
62 |
At this point your previous branch will be stashed and the working copy updated |
713.3.5
by Dave Cheney
final tweaks |
63 |
to match the state of the `add-CONTRIBUTING` branch. You must ensure any |
64 |
outstanding changes to the previous branch are committed or reverted to avoid |
|
65 |
local merge issues. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
66 |
|
67 |
You can also list any branches you are currently working on by |
|
68 |
||
69 |
bzr branch |
|
70 |
||
1408.1.1
by John Arbash Meinel
Start changing the imports of the middle level files. |
71 |
|
72 |
Imports
|
|
73 |
-------
|
|
74 |
||
75 |
Import statements are grouped into 3 sections: standard library, 3rd party |
|
76 |
libraries, juju-core imports. The tool "go fmt" can be used to ensure each |
|
77 |
group is alphabetically sorted. eg: |
|
78 |
||
79 |
import ( |
|
80 |
"fmt"
|
|
81 |
"time"
|
|
82 |
||
83 |
"labix.org/v2/mgo"
|
|
84 |
gc "launchpad.net/gocheck" |
|
85 |
"launchpad.net/loggo"
|
|
86 |
||
87 |
"launchpad.net/juju-core/state"
|
|
88 |
"launchpad.net/juju-core/worker"
|
|
89 |
)
|
|
90 |
||
91 |
Because "launchpad.net/gocheck" will be referenced frequently in test suites, |
|
92 |
its name gets a default short name of just "gc". |
|
93 |
||
713.3.1
by Dave Cheney
added CONTRIBUTING |
94 |
Testing
|
95 |
=======
|
|
96 |
||
713.3.4
by Dave Cheney
responding to review feedback |
97 |
`juju-core` uses the `gocheck` testing framework. `gocheck` is automatically |
98 |
installed as a dependency of `juju-core`. You can read more about `gocheck` |
|
99 |
at http://go.pkgdoc.org/pkg/launchpad.net/gocheck. `gocheck` is integrated |
|
100 |
into the source of each package so the standard `go test` command is used |
|
101 |
to run `gocheck` tests. For example |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
102 |
|
103 |
go test launchpad.net/juju-core/... |
|
104 |
||
713.3.4
by Dave Cheney
responding to review feedback |
105 |
will run all the tests in the `juju-core` project. By default `gocheck` prints |
106 |
only minimal output, and as `gocheck` is hooked into the testing framework via |
|
107 |
a single `go test` test per package, the usual `go test -v` flags are less |
|
108 |
useful. As a replacement the following commands produce more output from |
|
109 |
`gocheck`. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
110 |
|
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
111 |
go test -gocheck.v |
713.3.1
by Dave Cheney
added CONTRIBUTING |
112 |
|
713.3.4
by Dave Cheney
responding to review feedback |
113 |
is similar to `go test -v` and outputs the name of each test as it is run as |
114 |
well as any logging statements. It is important to note that these statements |
|
115 |
are buffered until the test completes. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
116 |
|
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
117 |
go test -gocheck.vv |
713.3.1
by Dave Cheney
added CONTRIBUTING |
118 |
|
713.3.4
by Dave Cheney
responding to review feedback |
119 |
extends the previous example by outputting any logging data immediately, rather |
120 |
than waiting for the test to complete. By default `gocheck` will run all tests |
|
121 |
in a package, selected tests can by run by passing `-gocheck.f` |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
122 |
|
123 |
go test -gocheck.f '$REGEX' |
|
124 |
||
125 |
to match a subset of test names. |
|
126 |
||
713.3.4
by Dave Cheney
responding to review feedback |
127 |
Finally, because by default `go test` runs the tests in the current package, and |
128 |
is not recursive, the following commands are equal, and will produce no output. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
129 |
|
826.1.1
by Dave Cheney
fix incorrect src path |
130 |
cd $GOPATH/src/launchpad.net/juju-core |
713.3.1
by Dave Cheney
added CONTRIBUTING |
131 |
go test |
132 |
||
133 |
go test launchpad.net/juju-core |
|
134 |
||
135 |
Proposing
|
|
136 |
=========
|
|
137 |
||
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
138 |
All code review is done on rietveld (http://code.google.com/p/rietveld/), not |
911.1.5
by Brad Crittenden
Suggested changes to documentation from review. |
139 |
on launchpad. |
713.3.5
by Dave Cheney
final tweaks |
140 |
|
141 |
Note: If this is your first time using `lbox` you will also be prompted to visit |
|
142 |
lauchpad to authorise an oauth token for `lbox` |
|
143 |
||
713.3.4
by Dave Cheney
responding to review feedback |
144 |
Once your change is ready, and you have successfully run all the tests you can |
145 |
propose your branch for merging |
|
146 |
||
147 |
lbox propose |
|
148 |
||
713.3.5
by Dave Cheney
final tweaks |
149 |
`lbox` will prompt you for a branch description and will create a rietveld code |
150 |
review for this change, linking it to the launchpad branch, and mailing |
|
151 |
reviewers. The `lbox` tool manages the state of the launchpad review, with the |
|
152 |
exception of marking reviews as rejected or work in progress by the reviewer. |
|
153 |
If you abandon a proposal, you should mark it as rejected in launchpad to avoid |
|
154 |
wasting the time of others. If you decide to start again, you should add a |
|
155 |
comment to the abandoned rietveld proposal linking it to your replacement. |
|
713.3.4
by Dave Cheney
responding to review feedback |
156 |
|
157 |
If your proposal requires additional work, then you can use the `lbox propose` |
|
158 |
command again to re-propose, this will also instruct rietveld to mail off any |
|
159 |
comments to your reviewers and upload fresh diff. |
|
160 |
||
161 |
If your branch requires another branch as a prerequisite use the `-req` flag to |
|
162 |
indicate so |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
163 |
|
164 |
lbox propose -req lp:dave-cheney/add-README |
|
165 |
||
713.3.4
by Dave Cheney
responding to review feedback |
166 |
This will produce a diff in rietveld which masks the changes from your prereq. |
713.3.1
by Dave Cheney
added CONTRIBUTING |
167 |
|
713.3.4
by Dave Cheney
responding to review feedback |
168 |
It is sometimes useful to be able to review your branch in rietveld without |
169 |
proposing, do to this the `-wip` flag is used |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
170 |
|
171 |
lbox propose -wip |
|
172 |
||
713.3.4
by Dave Cheney
responding to review feedback |
173 |
You can also edit the description of your change with `-edit`. This is useful |
174 |
when combined with the `-wip` flag to avoid sending too many mails to reviewers |
|
175 |
while preparing your proposal. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
176 |
|
177 |
lbox propose -edit |
|
178 |
||
713.3.5
by Dave Cheney
final tweaks |
179 |
By default, lbox creates a new bug in launchpad for each proposed branch, and |
180 |
assigns the branch to it. If you wish to assign it to an existing bug instead, |
|
181 |
use the `-bug` flag to link the branch inside launchpad. |
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
182 |
|
911.1.1
by Brad Crittenden
Move guts of get command to statecmd |
183 |
lbox propose -bug 1234567 |
713.3.1
by Dave Cheney
added CONTRIBUTING |
184 |
|
713.3.5
by Dave Cheney
final tweaks |
185 |
There is currently a bug with `lbox` when linking a branch to a bug which sets |
186 |
the milestone field to the last milestone defined on the project. Generally |
|
713.3.4
by Dave Cheney
responding to review feedback |
187 |
this is not what you want, so you should visit the bug's page and correct the |
188 |
milestone, if set.
|
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
189 |
|
190 |
Code review
|
|
191 |
===========
|
|
192 |
||
713.3.4
by Dave Cheney
responding to review feedback |
193 |
`juju-core` operates on a two positive, no negative review process. You may not
|
194 |
submit your proposal until it has received two LGTM comments. If any NOT LGTM
|
|
195 |
comments are received, those comments should be resolved to the satisfaction
|
|
196 |
of the objecting reviewer before submitting. Once your have received at least
|
|
1590.2.4
by Roger Peppe
include dependency info in CONTRIBUTING |
197 |
two positive reviews, you can submit your branch by going to the launchpad
|
198 |
merge proposal page and:
|
|
199 |
||
200 |
- copy and paste the merge proposal description into the
|
|
201 |
commit message.
|
|
202 |
- mark the proposal as Approved.
|
|
203 |
||
204 |
The merge proposal will then be tested and merged into trunk assuming
|
|
205 |
all tests pass cleanly.
|
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
206 |
|
207 |
lbox hooks
|
|
208 |
----------
|
|
209 |
||
713.3.4
by Dave Cheney
responding to review feedback |
210 |
Before proposing, `lbox` runs a number of hooks to improve code quality and
|
211 |
ensure that code is properly formatted. These checks are in
|
|
826.1.1
by Dave Cheney
fix incorrect src path |
212 |
`$GOPATH/src/launchpad.net/juju-core/.lbox.check`. They are run automatically
|
713.3.4
by Dave Cheney
responding to review feedback |
213 |
by `lbox` before proposing or submitting. If these hooks fail you will have
|
214 |
to resolve those errors before trying again. For example
|
|
713.3.1
by Dave Cheney
added CONTRIBUTING |
215 |
|
216 |
% lbox propose
|
|
217 |
gofmt is sad:
|
|
218 |
||
219 |
version/version.go
|
|
220 |
||
1590.2.4
by Roger Peppe
include dependency info in CONTRIBUTING |
221 |
Dependency management
|
222 |
=====================
|
|
223 |
||
224 |
In the top-level directory, there is a file, dependencies.tsv, that
|
|
225 |
holds the revision ids of all the external projects that juju-core
|
|
226 |
depends on. The tab-separated columns in the file are
|
|
227 |
the project name, the type version control system used by
|
|
228 |
that project, and the revision id and number respectively.
|
|
229 |
||
230 |
This file is generated by running the godeps command (which you
|
|
231 |
can get with `go get launchpad.net/godeps') on a juju-core |
|
232 |
installation with all freshly downloaded directories. |
|
233 |
||
234 |
The bash commands used to generate it from scratch are as follows: |
|
235 |
||
236 |
% export GOPATH=/tmp/juju-build |
|
237 |
% go get launchpad.net/juju-core/... |
|
238 |
% go test launchpad.net/juju-core/... |
|
239 |
% godeps -t $(go list launchpad.net/juju-core/...) > dependencies.tsv |