Merge pull request kubernetes#13278 from bgrant0607/docfix2

Start on expanding code expectations (aka "The bar")
nikhiljindal · Sep 4, 2015 · 58e9ee2 · 58e9ee2
2 parents e1bfce8 + 97e5058
commit 58e9ee2
Show file tree

Hide file tree

Showing 7 changed files with 199 additions and 15 deletions.
diff --git a/docs/devel/api-conventions.md b/docs/devel/api-conventions.md
@@ -713,6 +713,13 @@ Annotations have very different intended usage from labels. We expect them to be
 
 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/`.
 
+Other advice regarding use of labels, annotations, and other generic map keys by Kubernetes components and tools:
+  - Key names should be all lowercase, with words separated by dashes, such as `desired-replicas`
+  - Prefix the key with `kubernetes.io/` or `foo.kubernetes.io/`, preferably the latter if the label/annotation is specific to `foo`
+    - For instance, prefer `service-account.kubernetes.io/name` over `kubernetes.io/service-account.name`
+  - 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.
+
+
 
 <!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
 [![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/devel/api-conventions.md?pixel)]()

diff --git a/docs/devel/api_changes.md b/docs/devel/api_changes.md
@@ -189,17 +189,82 @@ API call might POST an object in API v7beta1 format, which uses the cleaner
 form (since v7beta1 is "beta").  When the user reads the object back in the
 v7beta1 API it would be unacceptable to have lost all but `Params[0]`.  This
 means that, even though it is ugly, a compatible change must be made to the v6
-API. However, this is very challenging to do correctly. It generally requires
+API.
+
+However, this is very challenging to do correctly. It often requires
 multiple representations of the same information in the same API resource, which
-need to be kept in sync in the event that either is changed. However, if
-the new representation is more expressive than the old, this breaks
-backward compatibility, since clients that only understood the old representation
+need to be kept in sync in the event that either is changed. For example,
+let's say you decide to rename a field within the same API version. In this case,
+you add units to `height` and `width`. You implement this by adding duplicate
+fields:
+
+```go
+type Frobber struct {
+	Height         *int          `json:"height"`
+	Width          *int          `json:"width"`
+	HeightInInches *int          `json:"heightInInches"`
+	WidthInInches  *int          `json:"widthInInches"`
+}
+```
+
+You convert all of the fields to pointers in order to distinguish between unset and
+set to 0, and then set each corresponding field from the other in the defaulting
+pass (e.g., `heightInInches` from `height`, and vice versa), which runs just prior
+to conversion. That works fine when the user creates a resource from a hand-written
+configuration -- clients can write either field and read either field, but what about
+creation or update from the output of GET, or update via PATCH (see
+[In-place updates](../user-guide/managing-deployments.md#in-place-updates-of-resources))?
+In this case, the two fields will conflict, because only one field would be updated
+in the case of an old client that was only aware of the old field (e.g., `height`).
+
+Say the client creates:
+
+```json
+{
+  "height": 10,
+  "width": 5
+}
+```
+
+and GETs:
+
+```json
+{
+  "height": 10,
+  "heightInInches": 10,
+  "width": 5,
+  "widthInInches": 5
+}
+```
+
+then PUTs back:
+
+```json
+{
+  "height": 13,
+  "heightInInches": 10,
+  "width": 5,
+  "widthInInches": 5
+}
+```
+
+The update should not fail, because it would have worked before `heightInInches` was added.
+
+Therefore, when there are duplicate fields, the old field MUST take precedence
+over the new, and the new field should be set to match by the server upon write.
+A new client would be aware of the old field as well as the new, and so can ensure
+that the old field is either unset or is set consistently with the new field. However,
+older clients would be unaware of the new field. Please avoid introducing duplicate
+fields due to the complexity they incur in the API.
+
+A new representation, even in a new API version, that is more expressive than an old one
+breaks backward compatibility, since clients that only understood the old representation
 would not be aware of the new representation nor its semantics. Examples of
 proposals that have run into this challenge include [generalized label
 selectors](http://issues.k8s.io/341) and [pod-level security
 context](http://prs.k8s.io/12823).
 
-As another interesting example, enumerated values provide a unique challenge.
+As another interesting example, enumerated values cause similar challenges.
 Adding a new value to an enumerated set is *not* a compatible change.  Clients
 which assume they know how to handle all possible values of a given field will
 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
 
 If you found that your change accidentally broke clients, it should be reverted.
 
+In short, the expected API evolution is as follows:
+* `experimental/v1alpha1` ->
+* `newapigroup/v1alpha1` -> ... -> `newapigroup/v1alphaN` ->
+* `newapigroup/v1beta1` -> ... -> `newapigroup/v1betaN` ->
+* `newapigroup/v1` ->
+* `newapigroup/v2alpha1` -> ...
+
+While in experimental we have no obligation to move forward with the API at all and may delete or break it at any time.
+
+While in alpha we expect to move forward with it, but may break it.
+
+Once in beta we will preserve forward compatibility, but may introduce new versions and delete old ones.
+
+v1 must be backward-compatible for an extended length of time.
+
 ## Changing versioned APIs
 
 For most changes, you will probably find it easiest to change the versioned

diff --git a/docs/devel/coding-conventions.md b/docs/devel/coding-conventions.md
@@ -30,12 +30,57 @@ Documentation for other releases can be found at
 <!-- END STRIP_FOR_RELEASE -->
 
 <!-- END MUNGE: UNVERSIONED_WARNING -->
-Coding style advice for contributors
+Code conventions
   - Bash
     - https://google-styleguide.googlecode.com/svn/trunk/shell.xml
+    - Ensure that build, release, test, and cluster-management scripts run on OS X
   - Go
-    - https://github.com/golang/go/wiki/CodeReviewComments
-    - https://gist.github.com/lavalamp/4bd23295a9f32706a48f
+    - Ensure your code passes the [presubmit checks](development.md#hooks)
+    - [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments)
+    - [Effective Go](https://golang.org/doc/effective_go.html)
+    - Comment your code.
+      - [Go's commenting conventions](http://blog.golang.org/godoc-documenting-go-code)
+      - If reviewers ask questions about why the code is the way it is, that's a sign that comments might be helpful.
+    - Command-line flags should use dashes, not underscores
+    - Naming
+      - Please consider package name when selecting an interface name, and avoid redundancy.
+        - e.g.: `storage.Interface` is better than `storage.StorageInterface`.
+      - Do not use uppercase characters, underscores, or dashes in package names.
+      - Please consider parent directory name when choosing a package name.
+        - so pkg/controllers/autoscaler/foo.go should say `package autoscaler` not `package autoscalercontroller`.
+        - Unless there's a good reason, the `package foo` line should match the name of the directory in which the .go file exists.
+        - Importers can use a different name if they need to disambiguate.
+    - API conventions
+      - [API changes](api_changes.md)
+      - [API conventions](api-conventions.md)
+    - [Kubectl conventions](kubectl-conventions.md)
+    - [Logging conventions](logging.md)
+
+Testing conventions
+  - All new packages and most new significant functionality must come with unit tests
+  - Table-driven tests are preferred for testing multiple scenarios/inputs; for example, see [TestNamespaceAuthorization](../../test/integration/auth_test.go)
+  - Significant features should come with integration (test/integration) and/or end-to-end (test/e2e) tests
+    - Including new kubectl commands and major features of existing commands
+  - 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).
+
+Directory and file conventions
+    - Avoid package sprawl. Find an appropriate subdirectory for new packages. (See [#4851](http://issues.k8s.io/4851) for discussion.)
+      - Libraries with no more appropriate home belong in new package subdirectories of pkg/util
+    - 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
+    - Go source files and directories use underscores, not dashes
+      - Package directories should generally avoid using separators as much as possible (when packages are multiple words, they usually should be in nested subdirectories).
+    - Document directories and filenames should use dashes rather than underscores
+    - 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.
+      - Examples should also illustrate [best practices for using the system](../user-guide/config-best-practices.md)
+    - Third-party code
+      - Third-party Go code is managed using Godeps
+      - Other third-party code belongs in /third_party
+      - Third-party code must include licenses
+      - This includes modified third-party code and excerpts, as well
+
+Coding advice
+  - Go
+    - [Go landmines](https://gist.github.com/lavalamp/4bd23295a9f32706a48f)
 
 
 <!-- BEGIN MUNGE: GENERATED_ANALYTICS -->

diff --git a/docs/devel/development.md b/docs/devel/development.md
@@ -112,6 +112,8 @@ fixups (e.g. automated doc formatting), use one or more commits for the
 changes to tooling and a final commit to apply the fixup en masse.  This makes
 reviews much easier.
 
+See [Faster Reviews](faster_reviews.md) for more details.
+
 ## godep and dependency management
 
 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``.

diff --git a/docs/devel/faster_reviews.md b/docs/devel/faster_reviews.md
@@ -53,15 +53,24 @@ later, just as soon as they have more free time (ha!).
 
 Let's talk about how to avoid this.
 
+## 0. Familiarize yourself with project conventions
+
+* [Development guide](development.md)
+* [Coding conventions](coding-conventions.md)
+* [API conventions](api-conventions.md)
+* [Kubectl conventions](kubectl-conventions.md)
+
 ## 1. Don't build a cathedral in one PR
 
 Are you sure FeatureX is something the Kubernetes team wants or will accept, or
 that it is implemented to fit with other changes in flight?  Are you willing to
 bet a few days or weeks of work on it?  If you have any doubt at all about the
-usefulness of your feature or the design - make a proposal doc or a sketch PR
-or both.  Write or code up just enough to express the idea and the design and
-why you made those choices, then get feedback on this.  Now, when we ask you to
-change a bunch of facets of the design, you don't have to re-write it all.
+usefulness of your feature or the design - make a proposal doc (in docs/proposals;
+for example [the QoS proposal](http://prs.k8s.io/11713)) or a sketch PR (e.g., just
+the API or Go interface) or both.  Write or code up just enough to express the idea
+and the design and why you made those choices, then get feedback on this. Be clear
+about what type of feedback you are asking for. Now, if we ask you to change a
+bunch of facets of the design, you won't have to re-write it all.
 
 ## 2. Smaller diffs are exponentially better
 
@@ -154,7 +163,20 @@ commit and re-push.  Your reviewer can then look at that commit on its own - so
 much faster to review than starting over.
 
 We might still ask you to clean up your commits at the very end, for the sake
-of a more readable history.
+of a more readable history, but don't do this until asked, typically at the point
+where the PR would otherwise be tagged LGTM.
+
+General squashing guidelines:
+
+* Sausage => squash
+
+  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.
+
+* Layers => don't squash
+
+  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.
+
+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.
 
 ## 8. KISS, YAGNI, MVP, etc
 

diff --git a/docs/devel/kubectl-conventions.md b/docs/devel/kubectl-conventions.md
@@ -34,7 +34,7 @@ Documentation for other releases can be found at
 Kubectl Conventions
 ===================
 
-Updated: 8/12/2015
+Updated: 8/27/2015
 
 **Table of Contents**
 <!-- BEGIN MUNGE: GENERATED_TOC -->
@@ -77,6 +77,31 @@ Updated: 8/12/2015
 * Flags are all lowercase, with words separated by hyphens
 * Flag names and single-character aliases should have the same meaning across all commands
 * Command-line flags corresponding to API fields should accept API enums exactly (e.g., --restart=Always)
+* 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
+* 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
+  * `-f`: Resource file
+    * also used for `--follow` in `logs`, but should be deprecated in favor of `-F`
+  * `-l`: Label selector
+    * also used for `--labels` in `expose`, but should be deprecated
+  * `-L`: Label columns
+  * `-c`: Container
+    * also used for `--client` in `version`, but should be deprecated
+  * `-i`: Attach stdin
+  * `-t`: Allocate TTY
+    * also used for `--template`, but deprecated
+  * `-w`: Watch (currently also used for `--www` in `proxy`, but should be deprecated)
+  * `-p`: Previous
+    * also used for `--pod` in `exec`, but deprecated
+    * also used for `--patch` in `patch`, but should be deprecated
+    * also used for `--port` in `proxy`, but should be deprecated
+  * `-P`: Static file prefix in `proxy`, but should be deprecated
+  * `-r`: Replicas
+  * `-u`: Unix socket
+  * `-v`: Verbose logging level
+* `--dry-run`: Don't modify the live state; simulate the mutation and display the output
+* `--local`: Don't contact the server; just do local read, transformation, generation, etc. and display the output
+* `--output-version=...`: Convert the output to a different API group/version
+* `--validate`: Validate the resource schema
 
 ## Output conventions
 

diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md
@@ -76,7 +76,10 @@ If you have what looks like a bug, or you would like to make a feature request,
 
 Before you file an issue, please search existing issues to see if your issue is already covered.
 
-If filing a bug, please include detailed information about how to reproduce the problem.
+If filing a bug, please include detailed information about how to reproduce the problem, such as:
+* Kubernetes version: `kubectl version`
+* Cloud provider, OS distro, network configuration, and Docker version
+* Steps to reproduce the problem
 
 <!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
 [![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/troubleshooting.md?pixel)]()