| ... |
... |
@@ -32,40 +32,31 @@ side effects and quirks the feature may have introduced. More on this below in |
|
32
|
32
|
|
|
33
|
33
|
.. _BuildGrid mailing list: https://lists.buildgrid.build/cgi-bin/mailman/listinfo/buildgrid
|
|
34
|
34
|
|
|
35
|
|
-
|
|
36
|
35
|
.. _patch-submissions:
|
|
37
|
36
|
|
|
38
|
37
|
Patch submissions
|
|
39
|
38
|
-----------------
|
|
40
|
39
|
|
|
41
|
|
-We are running `trunk based development`_. The idea behind this is that merge
|
|
42
|
|
-requests to the trunk will be small and made often, thus making the review and
|
|
43
|
|
-merge process as fast as possible. We do not want to end up with a huge backlog
|
|
44
|
|
-of outstanding merge requests. If possible, it is preferred that merge requests
|
|
45
|
|
-address specific points and clearly outline what problem they are solving.
|
|
46
|
|
-
|
|
47
|
|
-Branches must be submitted as merge requests (MR) on GitLab and should be
|
|
48
|
|
-associated with an issue, whenever possible. If it's a small change, we'll
|
|
49
|
|
-accept an MR without it being associated to an issue, but generally we prefer an
|
|
50
|
|
-issue to be raised in advance. This is so that we can track the work that is
|
|
|
40
|
+Branches must be submitted as merge requests (MR) on GitLab and should have a
|
|
|
41
|
+corresponding issue raised in advance (whenever possible). If it's a small change,
|
|
|
42
|
+an MR without it being associated to an issue is okay, but generally we prefer an
|
|
|
43
|
+issue to be raised in advance so that we can track the work that is
|
|
51
|
44
|
currently in progress on the project.
|
|
52
|
45
|
|
|
|
46
|
+When submitting a merge request, please obtain a review from another committer
|
|
|
47
|
+who is familiar with the area of the code base which the branch effects. An
|
|
|
48
|
+approval from another committer who is not the patch author will be needed
|
|
|
49
|
+before any merge (we use Gitlab's 'approval' feature for this).
|
|
|
50
|
+
|
|
53
|
51
|
Below is a list of good patch submission good practice:
|
|
54
|
52
|
|
|
55
|
53
|
- Each commit should address a specific issue number in the commit message. This
|
|
56
|
54
|
is really important for provenance reasons.
|
|
57
|
|
-- Merge requests that are not yet ready for review must be prefixed with the
|
|
58
|
|
- ``WIP:`` identifier, but if we stick to trunk based development then the
|
|
59
|
|
- ``WIP:`` identifier will not stay around for very long on a merge request.
|
|
60
|
|
-- When a merge request is ready for review, please find someone willing to do
|
|
61
|
|
- the review (ideally a maintainer) and assign them the MR, leaving a comment
|
|
62
|
|
- asking for their review.
|
|
|
55
|
+- Merge requests that are not yet ready for review should be prefixed with the
|
|
|
56
|
+ ``WIP:`` identifier.
|
|
63
|
57
|
- Submitted branches should not contain a history of work done.
|
|
64
|
58
|
- Unit tests should be a separate commit.
|
|
65
|
59
|
|
|
66
|
|
-.. _trunk based development: https://trunkbaseddevelopment.com
|
|
67
|
|
-
|
|
68
|
|
-
|
|
69
|
60
|
Commit messages
|
|
70
|
61
|
~~~~~~~~~~~~~~~
|
|
71
|
62
|
|
| ... |
... |
@@ -89,6 +80,63 @@ For more tips, please read `The seven rules of a great Git commit message`_. |
|
89
|
80
|
|
|
90
|
81
|
.. _The seven rules of a great Git commit message: https://chris.beams.io/posts/git-commit/#seven-rules
|
|
91
|
82
|
|
|
|
83
|
+.. _committer-access:
|
|
|
84
|
+
|
|
|
85
|
+Committer access
|
|
|
86
|
+----------------
|
|
|
87
|
+
|
|
|
88
|
+In the beginning of the project, we handed out commit access to anyone who had
|
|
|
89
|
+successfully landed a single patch to the code base. We then saw the policy in
|
|
|
90
|
+place over at the `Subversion`_ project and thought this was more sensible, so
|
|
|
91
|
+we decided to employ a very similar - although more light-weight - policy.
|
|
|
92
|
+
|
|
|
93
|
+Committers in the BuildGrid project are those folks to whom the right to
|
|
|
94
|
+directly commit changes to our version controlled resources has been granted.
|
|
|
95
|
+The project is meritocratic, which means (among other things) that project
|
|
|
96
|
+governance is handled by those who do the work. While every contribution is
|
|
|
97
|
+valued regardless of its source, not every person who contributes code to the
|
|
|
98
|
+project will earn commit access. The `COMMITTERS`_ file lists all committers.
|
|
|
99
|
+
|
|
|
100
|
+.. _COMMITTERS: https://gitlab.com/BuildGrid/buildgrid/blob/master/COMMITTERS.md
|
|
|
101
|
+.. _Subversion: http://subversion.apache.org/docs/community-guide/roles.html#committers
|
|
|
102
|
+
|
|
|
103
|
+
|
|
|
104
|
+How commit access is granted
|
|
|
105
|
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
|
|
106
|
+
|
|
|
107
|
+After someone has successfully contributed a few non-trivial patches, some full
|
|
|
108
|
+committer, usually whoever has reviewed and applied the most patches from that
|
|
|
109
|
+contributor, proposes them for commit access. This proposal is sent only to the
|
|
|
110
|
+other full committers -- the ensuing discussion is private, so that everyone can
|
|
|
111
|
+feel comfortable speaking their minds. Assuming there are no objections, the
|
|
|
112
|
+contributor is granted commit access. The decision is made by consensus; there
|
|
|
113
|
+are no formal rules governing the procedure, though generally if someone strongly
|
|
|
114
|
+objects the access is not offered, or is offered on a provisional basis.
|
|
|
115
|
+
|
|
|
116
|
+This of course relies on contributors being responsive and showing willingness
|
|
|
117
|
+to address any problems that may arise after landing patches. However, the primary
|
|
|
118
|
+criterion for commit access is good judgment.
|
|
|
119
|
+
|
|
|
120
|
+You do not have to be a technical wizard, or demonstrate deep knowledge of the
|
|
|
121
|
+entire codebase to become a committer. You just need to know what you don't
|
|
|
122
|
+know. If your patches adhere to the guidelines in this file, adhere to all the usual
|
|
|
123
|
+unquantifiable rules of coding (code should be readable, robust, maintainable, etc.),
|
|
|
124
|
+and respect the Hippocratic Principle of "first, do no harm", then you will probably
|
|
|
125
|
+get commit access pretty quickly. The size, complexity, and quantity of your patches
|
|
|
126
|
+do not matter as much as the degree of care you show in avoiding bugs and minimizing
|
|
|
127
|
+unnecessary impact on the rest of the code. Many full committers are people who have
|
|
|
128
|
+not made major code contributions, but rather lots of small, clean fixes, each of
|
|
|
129
|
+which was an unambiguous improvement to the code. (Of course, this does not mean the
|
|
|
130
|
+project needs a bunch of very trivial patches whose only purpose is to gain commit
|
|
|
131
|
+access; knowing what's worth a patch post and what's not is part of showing good
|
|
|
132
|
+judgement :-) .)
|
|
|
133
|
+
|
|
|
134
|
+When submitting a merge request, please obtain a review from another committer
|
|
|
135
|
+who is familiar with the area of the code base which the branch effects. Asking on
|
|
|
136
|
+slack is probably the best way to go about this. An approval from a committer
|
|
|
137
|
+who is not the patch author will be needed before any merge (we use Gitlab's 'approval'
|
|
|
138
|
+feature for this).
|
|
|
139
|
+
|
|
92
|
140
|
|
|
93
|
141
|
.. _coding-style:
|
|
94
|
142
|
|
| ... |
... |
@@ -198,35 +246,6 @@ trunk. |
|
198
|
246
|
|
|
199
|
247
|
.. _coverage report: https://buildgrid.gitlab.io/buildgrid/coverage/
|
|
200
|
248
|
|
|
201
|
|
-
|
|
202
|
|
-.. _committer-access:
|
|
203
|
|
-
|
|
204
|
|
-Committer access
|
|
205
|
|
-----------------
|
|
206
|
|
-
|
|
207
|
|
-We'll hand out commit access to anyone who has successfully landed a single
|
|
208
|
|
-patch to the code base. Please request this via Slack or the mailing list.
|
|
209
|
|
-
|
|
210
|
|
-This of course relies on contributors being responsive and showing willingness
|
|
211
|
|
-to address any problems that may arise after landing branches.
|
|
212
|
|
-
|
|
213
|
|
-When submitting a merge request, please obtain a review from another committer
|
|
214
|
|
-who is familiar with the area of the code base which the branch effects. An
|
|
215
|
|
-approval from another committer who is not the patch author will be needed
|
|
216
|
|
-before any merge (we use gitlab's 'approval' feature for this).
|
|
217
|
|
-
|
|
218
|
|
-What we are expecting of committers here in general is basically to escalate the
|
|
219
|
|
-review in cases of uncertainty.
|
|
220
|
|
-
|
|
221
|
|
-.. note::
|
|
222
|
|
-
|
|
223
|
|
- We don't have any detailed policy for "bad actors", but will of course handle
|
|
224
|
|
- things on a case by case basis - commit access should not result in commit
|
|
225
|
|
- wars or be used as a tool to subvert the project when disagreements arise.
|
|
226
|
|
- Such incidents (if any) would surely lead to temporary suspension of commit
|
|
227
|
|
- rights.
|
|
228
|
|
-
|
|
229
|
|
-
|
|
230
|
249
|
.. _gitlab-features:
|
|
231
|
250
|
|
|
232
|
251
|
GitLab features
|
