Merge pull request hashicorp#465 from hashicorp/resources-deprecation

Deprecate setting resources as string.
duckhan · May 28, 2020 · 627c7ae · 627c7ae
2 parents 15f8cab + f3044d1
commit 627c7ae
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,37 @@ BREAKING CHANGES:
   option was to work around an issue where mesh gateways would not listen on their
   bind ports until a Connect service was registered. This issue was fixed in Consul 1.6.2. ([GH-464](https://github.com/hashicorp/consul-helm/pull/464))
 
+DEPRECATIONS
+
+* Setting resources via YAML string is now deprecated. Instead, set directly as YAML.
+  This affects `client.resources`, `server.resources` and `meshGateway.resources`.
+  To set directly as YAML, simply remove the pipe (`|`) character that defines
+  the YAML as a string: 
+
+  Before:
+  ```yaml
+  client:
+    resources: |
+      requests:
+        memory: "128Mi"
+        cpu: "250m"
+      limits:
+        memory: "256Mi"
+        cpu: "500m"
+  ```
+  
+  After:
+  ```yaml
+  client:
+    resources:
+      requests:
+        memory: "128Mi"
+        cpu: "250m"
+      limits:
+        memory: "256Mi"
+        cpu: "500m"
+  ```
+
 ## 0.21.0 (May 14, 2020)
 
 FEATURES

diff --git a/templates/client-daemonset.yaml b/templates/client-daemonset.yaml
@@ -295,7 +295,11 @@ spec:
                   2>/dev/null | grep -E '".+"'
           {{- if .Values.client.resources }}
           resources:
+            {{- if eq (typeOf .Values.client.resources) "string" }}
             {{ tpl .Values.client.resources . | nindent 12 | trim }}
+            {{- else }}
+            {{- toYaml .Values.client.resources | nindent 12 }}
+            {{- end }}
           {{- end }}
       {{- if (or .Values.global.acls.manageSystemACLs .Values.global.bootstrapACLs (and .Values.global.tls.enabled (not .Values.global.tls.enableAutoEncrypt))) }}
       initContainers:

diff --git a/templates/mesh-gateway-deployment.yaml b/templates/mesh-gateway-deployment.yaml
@@ -212,7 +212,11 @@ spec:
           image: {{ .Values.meshGateway.imageEnvoy | quote }}
           {{- if .Values.meshGateway.resources }}
           resources:
+            {{- if eq (typeOf .Values.meshGateway.resources) "string" }}
             {{ tpl .Values.meshGateway.resources . | nindent 12 | trim }}
+            {{- else }}
+            {{- toYaml .Values.meshGateway.resources | nindent 12 }}
+            {{- end }}
           {{- end }}
           volumeMounts:
           - name: consul-bin

diff --git a/templates/server-statefulset.yaml b/templates/server-statefulset.yaml
@@ -252,7 +252,11 @@ spec:
             timeoutSeconds: 5
           {{- if .Values.server.resources }}
           resources:
+            {{- if eq (typeOf .Values.server.resources) "string" }}
             {{ tpl .Values.server.resources . | nindent 12 | trim }}
+            {{- else }}
+            {{- toYaml .Values.server.resources | nindent 12 }}
+            {{- end }}
           {{- end }}
       {{- if .Values.server.nodeSelector }}
       nodeSelector:

diff --git a/test/unit/client-daemonset.bats b/test/unit/client-daemonset.bats
@@ -163,10 +163,21 @@ load _helpers
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/client-daemonset.yaml  \
-      --set 'client.resources=foo' \
+      --set 'client.resources.foo=bar' \
       . | tee /dev/stderr |
-      yq -r '.spec.template.spec.containers[0].resources' | tee /dev/stderr)
-  [ "${actual}" = "foo" ]
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
+}
+
+# Test support for the deprecated method of setting a YAML string.
+@test "client/DaemonSet: resources can be set with string" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/client-daemonset.yaml  \
+      --set 'client.resources=foo: bar' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
 }
 
 #--------------------------------------------------------------------

diff --git a/test/unit/mesh-gateway-deployment.bats b/test/unit/mesh-gateway-deployment.bats
@@ -297,10 +297,24 @@ key2: value2' \
       --set 'meshGateway.enabled=true' \
       --set 'connectInject.enabled=true' \
       --set 'client.grpc=true' \
-      --set 'meshGateway.resources=requests: yadayada' \
+      --set 'meshGateway.resources.foo=bar' \
       . | tee /dev/stderr |
-      yq -r '.spec.template.spec.containers[0].resources.requests' | tee /dev/stderr)
-  [ "${actual}" = "yadayada" ]
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
+}
+
+# Test support for the deprecated method of setting a YAML string.
+@test "meshGateway/Deployment: resources can be overridden with string" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/mesh-gateway-deployment.yaml  \
+      --set 'meshGateway.enabled=true' \
+      --set 'connectInject.enabled=true' \
+      --set 'client.grpc=true' \
+      --set 'meshGateway.resources=foo: bar' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
 }
 
 #--------------------------------------------------------------------

diff --git a/test/unit/server-statefulset.bats b/test/unit/server-statefulset.bats
@@ -96,10 +96,21 @@ load _helpers
   cd `chart_dir`
   local actual=$(helm template \
       -x templates/server-statefulset.yaml  \
-      --set 'server.resources=foo' \
+      --set 'server.resources.foo=bar' \
       . | tee /dev/stderr |
-      yq -r '.spec.template.spec.containers[0].resources' | tee /dev/stderr)
-  [ "${actual}" = "foo" ]
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
+}
+
+# Test support for the deprecated method of setting a YAML string.
+@test "server/StatefulSet: resources can be set with string" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -x templates/server-statefulset.yaml  \
+      --set 'server.resources=foo: bar' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.containers[0].resources.foo' | tee /dev/stderr)
+  [ "${actual}" = "bar" ]
 }
 
 #--------------------------------------------------------------------

diff --git a/values.yaml b/values.yaml
@@ -252,9 +252,10 @@ server:
   connect: true
 
   # Resource requests, limits, etc. for the server cluster placement. This
-  # should map directly to the value of the resources field for a PodSpec,
-  # formatted as a multi-line string. By default no direct resource request
-  # is made.
+  # should map directly to the value of the resources field for a PodSpec.
+  # By default no direct resource request is made.
+  # NOTE: The use of a YAML string is deprecated. Instead, set directly as a
+  #       YAML map.
   resources: null
 
   # updatePartition is used to control a careful rolling update of Consul
@@ -411,9 +412,10 @@ client:
   exposeGossipPorts: false
 
   # Resource requests, limits, etc. for the client cluster placement. This
-  # should map directly to the value of the resources field for a PodSpec,
-  # formatted as a multi-line string. By default no direct resource request
-  # is made.
+  # should map directly to the value of the resources field for a PodSpec.
+  # By default no direct resource request is made.
+  # NOTE: The use of a YAML string is deprecated. Instead, set directly as a
+  #       YAML map.
   resources: null
 
   # extraConfig is a raw string of extra configuration to set with the
@@ -963,7 +965,10 @@ meshGateway:
   # agent.
   hostPort: null
 
-  resources: |
+  # Resource settings for mesh gateway pods.
+  # NOTE: The use of a YAML string is deprecated. Instead, set directly as a
+  #       YAML map.
+  resources:
     requests:
       memory: "128Mi"
       cpu: "250m"