Skip to content

Commit 58e9ee2

Browse files
committed
Merge pull request kubernetes#13278 from bgrant0607/docfix2
Start on expanding code expectations (aka "The bar")
2 parents e1bfce8 + 97e5058 commit 58e9ee2

7 files changed

+199
-15
lines changed

docs/devel/api-conventions.md

+7
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,13 @@ Annotations have very different intended usage from labels. We expect them to be
713713

714714
In fact, experimental API fields, including to represent fields of newer alpha/beta API versions in the older, stable storage version, may be represented as annotations with the prefix `experimental.kubernetes.io/`.
715715

716+
Other advice regarding use of labels, annotations, and other generic map keys by Kubernetes components and tools:
717+
- Key names should be all lowercase, with words separated by dashes, such as `desired-replicas`
718+
- Prefix the key with `kubernetes.io/` or `foo.kubernetes.io/`, preferably the latter if the label/annotation is specific to `foo`
719+
- For instance, prefer `service-account.kubernetes.io/name` over `kubernetes.io/service-account.name`
720+
- Use annotations to store API extensions that the controller responsible for the resource doesn't need to know about, experimental fields that aren't intended to be generally used API fields, etc. Beware that annotations aren't automatically handled by the API conversion machinery.
721+
722+
716723

717724
<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
718725
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/devel/api-conventions.md?pixel)]()

docs/devel/api_changes.md

+85-5
Original file line numberDiff line numberDiff line change
@@ -189,17 +189,82 @@ API call might POST an object in API v7beta1 format, which uses the cleaner
189189
form (since v7beta1 is "beta"). When the user reads the object back in the
190190
v7beta1 API it would be unacceptable to have lost all but `Params[0]`. This
191191
means that, even though it is ugly, a compatible change must be made to the v6
192-
API. However, this is very challenging to do correctly. It generally requires
192+
API.
193+
194+
However, this is very challenging to do correctly. It often requires
193195
multiple representations of the same information in the same API resource, which
194-
need to be kept in sync in the event that either is changed. However, if
195-
the new representation is more expressive than the old, this breaks
196-
backward compatibility, since clients that only understood the old representation
196+
need to be kept in sync in the event that either is changed. For example,
197+
let's say you decide to rename a field within the same API version. In this case,
198+
you add units to `height` and `width`. You implement this by adding duplicate
199+
fields:
200+
201+
```go
202+
type Frobber struct {
203+
Height *int `json:"height"`
204+
Width *int `json:"width"`
205+
HeightInInches *int `json:"heightInInches"`
206+
WidthInInches *int `json:"widthInInches"`
207+
}
208+
```
209+
210+
You convert all of the fields to pointers in order to distinguish between unset and
211+
set to 0, and then set each corresponding field from the other in the defaulting
212+
pass (e.g., `heightInInches` from `height`, and vice versa), which runs just prior
213+
to conversion. That works fine when the user creates a resource from a hand-written
214+
configuration -- clients can write either field and read either field, but what about
215+
creation or update from the output of GET, or update via PATCH (see
216+
[In-place updates](../user-guide/managing-deployments.md#in-place-updates-of-resources))?
217+
In this case, the two fields will conflict, because only one field would be updated
218+
in the case of an old client that was only aware of the old field (e.g., `height`).
219+
220+
Say the client creates:
221+
222+
```json
223+
{
224+
"height": 10,
225+
"width": 5
226+
}
227+
```
228+
229+
and GETs:
230+
231+
```json
232+
{
233+
"height": 10,
234+
"heightInInches": 10,
235+
"width": 5,
236+
"widthInInches": 5
237+
}
238+
```
239+
240+
then PUTs back:
241+
242+
```json
243+
{
244+
"height": 13,
245+
"heightInInches": 10,
246+
"width": 5,
247+
"widthInInches": 5
248+
}
249+
```
250+
251+
The update should not fail, because it would have worked before `heightInInches` was added.
252+
253+
Therefore, when there are duplicate fields, the old field MUST take precedence
254+
over the new, and the new field should be set to match by the server upon write.
255+
A new client would be aware of the old field as well as the new, and so can ensure
256+
that the old field is either unset or is set consistently with the new field. However,
257+
older clients would be unaware of the new field. Please avoid introducing duplicate
258+
fields due to the complexity they incur in the API.
259+
260+
A new representation, even in a new API version, that is more expressive than an old one
261+
breaks backward compatibility, since clients that only understood the old representation
197262
would not be aware of the new representation nor its semantics. Examples of
198263
proposals that have run into this challenge include [generalized label
199264
selectors](http://issues.k8s.io/341) and [pod-level security
200265
context](http://prs.k8s.io/12823).
201266

202-
As another interesting example, enumerated values provide a unique challenge.
267+
As another interesting example, enumerated values cause similar challenges.
203268
Adding a new value to an enumerated set is *not* a compatible change. Clients
204269
which assume they know how to handle all possible values of a given field will
205270
not be able to handle the new values. However, removing value from an
@@ -227,6 +292,21 @@ the release notes for the next release by labeling the PR with the "release-note
227292

228293
If you found that your change accidentally broke clients, it should be reverted.
229294

295+
In short, the expected API evolution is as follows:
296+
* `experimental/v1alpha1` ->
297+
* `newapigroup/v1alpha1` -> ... -> `newapigroup/v1alphaN` ->
298+
* `newapigroup/v1beta1` -> ... -> `newapigroup/v1betaN` ->
299+
* `newapigroup/v1` ->
300+
* `newapigroup/v2alpha1` -> ...
301+
302+
While in experimental we have no obligation to move forward with the API at all and may delete or break it at any time.
303+
304+
While in alpha we expect to move forward with it, but may break it.
305+
306+
Once in beta we will preserve forward compatibility, but may introduce new versions and delete old ones.
307+
308+
v1 must be backward-compatible for an extended length of time.
309+
230310
## Changing versioned APIs
231311

232312
For most changes, you will probably find it easiest to change the versioned

docs/devel/coding-conventions.md

+48-3
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,57 @@ Documentation for other releases can be found at
3030
<!-- END STRIP_FOR_RELEASE -->
3131

3232
<!-- END MUNGE: UNVERSIONED_WARNING -->
33-
Coding style advice for contributors
33+
Code conventions
3434
- Bash
3535
- https://google-styleguide.googlecode.com/svn/trunk/shell.xml
36+
- Ensure that build, release, test, and cluster-management scripts run on OS X
3637
- Go
37-
- https://github.com/golang/go/wiki/CodeReviewComments
38-
- https://gist.github.com/lavalamp/4bd23295a9f32706a48f
38+
- Ensure your code passes the [presubmit checks](development.md#hooks)
39+
- [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
40+
- [Effective Go](https://golang.org/doc/effective_go.html)
41+
- Comment your code.
42+
- [Go's commenting conventions](http://blog.golang.org/godoc-documenting-go-code)
43+
- If reviewers ask questions about why the code is the way it is, that's a sign that comments might be helpful.
44+
- Command-line flags should use dashes, not underscores
45+
- Naming
46+
- Please consider package name when selecting an interface name, and avoid redundancy.
47+
- e.g.: `storage.Interface` is better than `storage.StorageInterface`.
48+
- Do not use uppercase characters, underscores, or dashes in package names.
49+
- Please consider parent directory name when choosing a package name.
50+
- so pkg/controllers/autoscaler/foo.go should say `package autoscaler` not `package autoscalercontroller`.
51+
- Unless there's a good reason, the `package foo` line should match the name of the directory in which the .go file exists.
52+
- Importers can use a different name if they need to disambiguate.
53+
- API conventions
54+
- [API changes](api_changes.md)
55+
- [API conventions](api-conventions.md)
56+
- [Kubectl conventions](kubectl-conventions.md)
57+
- [Logging conventions](logging.md)
58+
59+
Testing conventions
60+
- All new packages and most new significant functionality must come with unit tests
61+
- Table-driven tests are preferred for testing multiple scenarios/inputs; for example, see [TestNamespaceAuthorization](../../test/integration/auth_test.go)
62+
- Significant features should come with integration (test/integration) and/or end-to-end (test/e2e) tests
63+
- Including new kubectl commands and major features of existing commands
64+
- Unit tests must pass on OS X and Windows platforms - if you use Linux specific features, your test case must either be skipped on windows or compiled out (skipped is better when running Linux specific commands, compiled out is required when your code does not compile on Windows).
65+
66+
Directory and file conventions
67+
- Avoid package sprawl. Find an appropriate subdirectory for new packages. (See [#4851](http://issues.k8s.io/4851) for discussion.)
68+
- Libraries with no more appropriate home belong in new package subdirectories of pkg/util
69+
- Avoid general utility packages. Packages called "util" are suspect. Instead, derive a name that describes your desired function. For example, the utility functions dealing with waiting for operations are in the "wait" package and include functionality like Poll. So the full name is wait.Poll
70+
- Go source files and directories use underscores, not dashes
71+
- Package directories should generally avoid using separators as much as possible (when packages are multiple words, they usually should be in nested subdirectories).
72+
- Document directories and filenames should use dashes rather than underscores
73+
- Contrived examples that illustrate system features belong in /docs/user-guide or /docs/admin, depending on whether it is a feature primarily intended for users that deploy applications or cluster administrators, respectively. Actual application examples belong in /examples.
74+
- Examples should also illustrate [best practices for using the system](../user-guide/config-best-practices.md)
75+
- Third-party code
76+
- Third-party Go code is managed using Godeps
77+
- Other third-party code belongs in /third_party
78+
- Third-party code must include licenses
79+
- This includes modified third-party code and excerpts, as well
80+
81+
Coding advice
82+
- Go
83+
- [Go landmines](https://gist.github.com/lavalamp/4bd23295a9f32706a48f)
3984

4085

4186
<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->

docs/devel/development.md

+2
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,8 @@ fixups (e.g. automated doc formatting), use one or more commits for the
112112
changes to tooling and a final commit to apply the fixup en masse. This makes
113113
reviews much easier.
114114

115+
See [Faster Reviews](faster_reviews.md) for more details.
116+
115117
## godep and dependency management
116118

117119
Kubernetes uses [godep](https://github.com/tools/godep) to manage dependencies. It is not strictly required for building Kubernetes but it is required when managing dependencies under the Godeps/ tree, and is required by a number of the build and test scripts. Please make sure that ``godep`` is installed and in your ``$PATH``.

docs/devel/faster_reviews.md

+27-5
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,24 @@ later, just as soon as they have more free time (ha!).
5353

5454
Let's talk about how to avoid this.
5555

56+
## 0. Familiarize yourself with project conventions
57+
58+
* [Development guide](development.md)
59+
* [Coding conventions](coding-conventions.md)
60+
* [API conventions](api-conventions.md)
61+
* [Kubectl conventions](kubectl-conventions.md)
62+
5663
## 1. Don't build a cathedral in one PR
5764

5865
Are you sure FeatureX is something the Kubernetes team wants or will accept, or
5966
that it is implemented to fit with other changes in flight? Are you willing to
6067
bet a few days or weeks of work on it? If you have any doubt at all about the
61-
usefulness of your feature or the design - make a proposal doc or a sketch PR
62-
or both. Write or code up just enough to express the idea and the design and
63-
why you made those choices, then get feedback on this. Now, when we ask you to
64-
change a bunch of facets of the design, you don't have to re-write it all.
68+
usefulness of your feature or the design - make a proposal doc (in docs/proposals;
69+
for example [the QoS proposal](http://prs.k8s.io/11713)) or a sketch PR (e.g., just
70+
the API or Go interface) or both. Write or code up just enough to express the idea
71+
and the design and why you made those choices, then get feedback on this. Be clear
72+
about what type of feedback you are asking for. Now, if we ask you to change a
73+
bunch of facets of the design, you won't have to re-write it all.
6574

6675
## 2. Smaller diffs are exponentially better
6776

@@ -154,7 +163,20 @@ commit and re-push. Your reviewer can then look at that commit on its own - so
154163
much faster to review than starting over.
155164

156165
We might still ask you to clean up your commits at the very end, for the sake
157-
of a more readable history.
166+
of a more readable history, but don't do this until asked, typically at the point
167+
where the PR would otherwise be tagged LGTM.
168+
169+
General squashing guidelines:
170+
171+
* Sausage => squash
172+
173+
When there are several commits to fix bugs in the original commit(s), address reviewer feedback, etc. Really we only want to see the end state and commit message for the whole PR.
174+
175+
* Layers => don't squash
176+
177+
When there are independent changes layered upon each other to achieve a single goal. For instance, writing a code munger could be one commit, applying it could be another, and adding a precommit check could be a third. One could argue they should be separate PRs, but there's really no way to test/review the munger without seeing it applied, and there needs to be a precommit check to ensure the munged output doesn't immediately get out of date.
178+
179+
A commit, as much as possible, should be a single logical change. Each commit should always have a good title line (<70 characters) and include an additional description paragraph describing in more detail the change intended. Do not link pull requests by `#` in a commit description, because GitHub creates lots of spam. Instead, reference other PRs via the PR your commit is in.
158180

159181
## 8. KISS, YAGNI, MVP, etc
160182

docs/devel/kubectl-conventions.md

+26-1
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ Documentation for other releases can be found at
3434
Kubectl Conventions
3535
===================
3636

37-
Updated: 8/12/2015
37+
Updated: 8/27/2015
3838

3939
**Table of Contents**
4040
<!-- BEGIN MUNGE: GENERATED_TOC -->
@@ -77,6 +77,31 @@ Updated: 8/12/2015
7777
* Flags are all lowercase, with words separated by hyphens
7878
* Flag names and single-character aliases should have the same meaning across all commands
7979
* Command-line flags corresponding to API fields should accept API enums exactly (e.g., --restart=Always)
80+
* Do not reuse flags for different semantic purposes, and do not use different flag names for the same semantic purpose -- grep for `"Flags()"` before adding a new flag
81+
* Use short flags sparingly, only for the most frequently used options, prefer lowercase over uppercase for the most common cases, try to stick to well known conventions for UNIX commands and/or Docker, where they exist, and update this list when adding new short flags
82+
* `-f`: Resource file
83+
* also used for `--follow` in `logs`, but should be deprecated in favor of `-F`
84+
* `-l`: Label selector
85+
* also used for `--labels` in `expose`, but should be deprecated
86+
* `-L`: Label columns
87+
* `-c`: Container
88+
* also used for `--client` in `version`, but should be deprecated
89+
* `-i`: Attach stdin
90+
* `-t`: Allocate TTY
91+
* also used for `--template`, but deprecated
92+
* `-w`: Watch (currently also used for `--www` in `proxy`, but should be deprecated)
93+
* `-p`: Previous
94+
* also used for `--pod` in `exec`, but deprecated
95+
* also used for `--patch` in `patch`, but should be deprecated
96+
* also used for `--port` in `proxy`, but should be deprecated
97+
* `-P`: Static file prefix in `proxy`, but should be deprecated
98+
* `-r`: Replicas
99+
* `-u`: Unix socket
100+
* `-v`: Verbose logging level
101+
* `--dry-run`: Don't modify the live state; simulate the mutation and display the output
102+
* `--local`: Don't contact the server; just do local read, transformation, generation, etc. and display the output
103+
* `--output-version=...`: Convert the output to a different API group/version
104+
* `--validate`: Validate the resource schema
80105

81106
## Output conventions
82107

docs/troubleshooting.md

+4-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,10 @@ If you have what looks like a bug, or you would like to make a feature request,
7676

7777
Before you file an issue, please search existing issues to see if your issue is already covered.
7878

79-
If filing a bug, please include detailed information about how to reproduce the problem.
79+
If filing a bug, please include detailed information about how to reproduce the problem, such as:
80+
* Kubernetes version: `kubectl version`
81+
* Cloud provider, OS distro, network configuration, and Docker version
82+
* Steps to reproduce the problem
8083

8184
<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
8285
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/troubleshooting.md?pixel)]()

0 commit comments

Comments
 (0)