Skip to content

Commit

Permalink
Guidelines: set matchLabels as being mandatory (helm#7692)
Browse files Browse the repository at this point in the history
* Guidelines: set matchLabels as being mandatory.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* fixup! Guidelines: set matchLabels as being mandatory.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* fixup! fixup! Guidelines: set matchLabels as being mandatory.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* Update guidelines: mention DaemonSets as well.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* Review guidelines: be more precise + specify upgrade

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* Review guidelines: fix typos, add persistence paragraph and do not repeat component part.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* Review guidelines: add PVC paragraph.

Signed-off-by: Cédric de Saint Martin <cdesaintmartin@wiremind.fr>

* Fix spelling/typos

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>

* Fix incorrect typo fix

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
  • Loading branch information
desaintmartin authored and kenichi-shibata committed Oct 30, 2018
1 parent 8173abc commit 291cabe
Showing 1 changed file with 56 additions and 3 deletions.
59 changes: 56 additions & 3 deletions REVIEW_GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ Stable charts should not depend on charts in incubator.

## Names and Labels

Resources and labels should follow some conventions. The standard resource metadata should be this:
### Metadata
Resources and labels should follow some conventions. The standard resource metadata (`metadata.labels` and `spec.template.metadata.labels`) should be this:

```yaml
name: {{ template "myapp.fullname" . }}
Expand All @@ -44,8 +45,41 @@ labels:
heritage: {{ .Release.Service }}
```
If a chart has multiple components, a `component` label should be added (e. g. `component: server`). The resource name should get the component as suffix (e. g. `name: {{ template "myapp.fullname" . }}-server`).

Note that templates have to be namespaced. With Helm 2.7+, `helm create` does this out-of-the-box. The `app` label should use the `name` template, not `fullname` as is still the case with older charts.

### Deployments, StatefulSets, DaemonSets Selectors

`spec.selector.matchLabels` must be specified should follow some conventions. The standard selector should be this:

```yaml
selector:
matchLabels:
app: {{ template "myapp.name" . }}
release: {{ .Release.Name }}
```

If a chart has multiple components, a `component` label should be added to the selector (see above).

`spec.selector.matchLabels` defined in `Deployments`/`StatefulSets`/`DaemonSets` `>=v1/beta2` **must not** contain `chart` label or any label containing a version of the chart, because the selector is immutable.
The chart label string contains the version, so if it is specified, whenever the the Chart.yaml version changes, Helm's attempt to change this immutable field would cause the upgrade to fail.

#### Fixing Selectors

##### For Deployments, StatefulSets, DaemonSets apps/v1beta1 or extensions/v1beta1

- If it does not specify `spec.selector.matchLabels`, set it
- Remove `chart` label in `spec.selector.matchLabels` if it exists
- Bump patch version of the Chart

##### For Deployments, StatefulSets, DaemonSets >=apps/v1beta2

- Remove `chart` label in `spec.selector.matchLabels` if it exists
- Bump major version of the Chart as it is a breaking change

### Service Selectors

Label selectors for services must have both `app` and `release` labels.

```yaml
Expand All @@ -54,7 +88,26 @@ selector:
release: {{ .Release.Name }}
```

If a chart has multiple components, a `component` label should be added (e. g. `component: server`). The resource name should get the component as suffix (e. g. `name: {{ template "myapp.fullname" . }}-server`). The `component` label must be added to label selectors as well.
If a chart has multiple components, a `component` label should be added to the selector (see above).

### Persistence Labels

### StatefulSet

In case of a `Statefulset`, `spec.volumeClaimTemplates.metadata.labels` must have both `app` and `release` labels, and **must not** contain `chart` label or any label containing a version of the chart, because `spec.volumeClaimTemplates` is immutable.

```yaml
labels:
app: {{ template "myapp.name" . }}
release: {{ .Release.Name }}
```

If a chart has multiple components, a `component` label should be added to the selector (see above).

### PersistentVolumeClaim

In case of a `PersistentVolumeClaim`, unless special needs, `matchLabels` should not be specified
because it would prevent automatic `PersistentVolume` provisioning.

## Formatting

Expand Down Expand Up @@ -269,7 +322,7 @@ spec:
We officially support compatibility with the current and the previous minor version of Kubernetes. Generated resources should use the latest possible API versions compatible with these versions. For extended backwards compatibility conditional logic based on capabilities may be used (see [built-in objects](https://github.com/helm/helm/blob/master/docs/chart_template_guide/builtin_objects.md)).
## Kubernetes Native Workloads.
## Kubernetes Native Workloads
While reviewing Charts that contain workloads such as [Deployments](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/), [StatefulSets](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/), [DaemonSets](https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/) and [Jobs](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) the below points should be considered. These are to be seen as best practices rather than strict enforcement.
Expand Down

0 comments on commit 291cabe

Please sign in to comment.