villebro commented on code in PR #207:
URL:
https://github.com/apache/superset-kubernetes-operator/pull/207#discussion_r3631512813
##########
charts/superset-operator/templates/deployment.yaml:
##########
@@ -22,6 +22,7 @@ metadata:
{{- include "superset-operator.labels" . | nindent 4 }}
spec:
replicas: {{ .Values.replicas }}
+ revisionHistoryLimit: {{ .Values.revisionHistoryLimit }}
Review Comment:
Let's make this conditional on `revisionHistoryLimit` actually being set:
```suggestion
{{- if not (kindIs "invalid" .Values.revisionHistoryLimit) }}
revisionHistoryLimit: {{ .Values.revisionHistoryLimit }}
{{- end }}
```
##########
charts/superset-operator/values.yaml:
##########
@@ -9,6 +9,9 @@ image:
# -- Number of operator manager replicas.
replicas: 1
+# -- Number of old ReplicaSets to retain for rollback (Kubernetes default 10).
+revisionHistoryLimit: 10
Review Comment:
Let's default to the property being unset, and refrain from repeating the
current default value (you never know if it will change some day). Admins will
be well aware of the default (and annoying 😆) default value.
```suggestion
# -- Number of old ReplicaSets to retain for rollback. Leave unset to use
the Kubernetes default. Set to 0 to disable rollback history.
revisionHistoryLimit: ~
```
##########
docs/reference/releases.md:
##########
@@ -25,6 +25,7 @@ This page tracks notable changes in Apache Superset
Kubernetes Operator releases
### Added
+- **Helm `revisionHistoryLimit`.** The Helm chart now exposes a
`revisionHistoryLimit` value (default `10`) that sets
`spec.revisionHistoryLimit` on the operator Deployment, capping the number of
old ReplicaSets retained for rollback ([@younsl](https://github.com/younsl)).
Review Comment:
Let's remove the explicit callout of current defaults. Something like this:
```suggestion
- **Helm `revisionHistoryLimit`.** The Helm chart now exposes a
`revisionHistoryLimit` value that allows setting `spec.revisionHistoryLimit` on
the operator Deployment, capping the number of old ReplicaSets retained for
rollback ([@younsl](https://github.com/younsl)).
```
##########
charts/superset-operator/values.schema.json:
##########
@@ -23,6 +23,7 @@
}
},
"replicas": { "type": "integer", "minimum": 0 },
+ "revisionHistoryLimit": { "type": "integer", "minimum": 0 },
Review Comment:
Let's accept null
```suggestion
"revisionHistoryLimit": { "type": ["integer", "null"], "minimum": 0 },
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
