Further review comments

GoogleContainerTools · May 27, 2020 · a8f39b5 · a8f39b5
1 parent a6ce136
commit a8f39b5
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 62 deletions.
diff --git a/tests/e2e.sh b/tests/e2e.sh
@@ -28,7 +28,6 @@ metadata:
 items: []
 EOF
 )
-HELM_ERROR_SNIPPET="Helm template command results in error"
 CHARTS_SRC="charts/bitnami"
 
 ############################
@@ -177,48 +176,6 @@ assert_dir_exists payments-dev
 assert_dir_exists payments-prod
 grep -q allowPrivilegeEscalation podsecuritypolicy_psp.yaml
 
-helm_testcase "docker_helm_template_undefined_args"
-docker run -u "$(id -u)" -v "$(pwd)/${CHARTS_SRC}":/source gcr.io/kpt-functions/helm-template:"${TAG}" -i /dev/null 2>err.txt || true
-assert_contains_string err.txt "Error: functionConfig expected, instead undefined"
-
-helm_testcase "docker_helm_template_empty_fc"
-cat >fc.yaml <<EOF
-apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: empty-config
-  annotations:
-    config.k8s.io/function: |
-      container:
-        image:  gcr.io/kpt-functions/helm-template
-    config.kubernetes.io/local-config: "true"
-data:
-EOF
-docker run -u "$(id -u)" -v "$(pwd)":/source gcr.io/kpt-functions/helm-template:"${TAG}" -i /dev/null -f /source/fc.yaml 2>err.txt || true
-assert_contains_string err.txt "Error: functionConfig expected to contain data, instead empty"
-
-helm_testcase "docker_helm_template_invalid_fc"
-cat >fc.yaml <<EOF
-apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: invalid-config
-  annotations:
-    config.k8s.io/function: |
-      container:
-        image:  gcr.io/kpt-functions/helm-template
-    config.kubernetes.io/local-config: "true"
-data:
-  name: invalid-fc
-  chart_path: /path/to/helm/chart
-EOF
-docker run -u "$(id -u)" -v "$(pwd)":/source gcr.io/kpt-functions/helm-template:"${TAG}" -i /dev/null -f /source/fc.yaml >out.yaml || true
-assert_contains_string out.yaml "${HELM_ERROR_SNIPPET}"
-
-helm_testcase "docker_helm_template_too_few_args"
-docker run -u "$(id -u)" -v "$(pwd)/${CHARTS_SRC}":/source gcr.io/kpt-functions/helm-template:"${TAG}" -i /dev/null -d name=too-few-args >out.yaml || true
-assert_contains_string out.yaml "${HELM_ERROR_SNIPPET}"
-
 helm_testcase "docker_helm_template_expected_args"
 docker run -u "$(id -u)" -v "$(pwd)/${CHARTS_SRC}":/source gcr.io/kpt-functions/helm-template:"${TAG}" -i /dev/null -d name=expected-args -d chart_path=/source/redis >out.yaml
 assert_contains_string out.yaml "expected-args"

diff --git a/ts/demo-functions/build/helm_template.Dockerfile b/ts/demo-functions/build/helm_template.Dockerfile
@@ -46,6 +46,6 @@ COPY --from=builder /home/node/app /home/node/app
 COPY --from=builder /usr/local/bin /usr/local/bin
 
 ENV PATH /usr/local/bin:$PATH
-ENV HELM_PATH_CACHE /tmp
+ENV HELM_PATH_CACHE /var/cache
 
 ENTRYPOINT ["node", "/home/node/app/dist/helm_template_run.js"]
diff --git a/ts/demo-functions/src/helm_template.ts b/ts/demo-functions/src/helm_template.ts
@@ -53,26 +53,28 @@ export async function helmTemplate(configs: Configs) {
 
 function readArguments(configs: Configs) {
   const args: string[] = [];
-
-  // Helm template expects name then chart path then remaining flags
-  const name = configs.getFunctionConfigValue(CHART_NAME);
-  if (name) {
-    args.push(name);
-  }
-  const chartPath = configs.getFunctionConfigValue(CHART_PATH);
-  if (chartPath) {
-    args.push(chartPath);
-  }
-
-  // Remaining flags
+  let nameArg;
+  let pathArg;
   const data = readConfigDataOrThrow(configs);
   for (const key in data) {
-    if (key !== CHART_NAME && key !== CHART_PATH) {
+    if (key === CHART_NAME) {
+      nameArg = data[key];
+    } else if (key === CHART_PATH) {
+      pathArg = data[key];
+    } else {
       args.push(key);
       args.push(data[key]);
     }
   }
 
+  // Helm template expects name and chart path first so place those at the beginning
+  if (pathArg) {
+    args.unshift(pathArg);
+  }
+  if (nameArg) {
+    args.unshift(nameArg);
+  }
+
   return args;
 }
 
@@ -95,12 +97,11 @@ function readConfigDataOrThrow(configs: Configs) {
 }
 
 helmTemplate.usage = `
-Render chart templates locally using helm template. If piped a Kubernetes List in
-addition to arguments then render the chart objects into the piped list,
-overwriting any chart objects that already exist in the list.
+Render chart templates locally using helm template. If input a list of configs in
+addition to arguments will overwrite any chart objects that already exist in the list.
 
-Configured using a ConfigMap with keys for ${CHART_NAME}, ${CHART_PATH}, and optional helm
-template flags like --values:
+Configured using a ConfigMap with keys for ${CHART_NAME}, ${CHART_PATH}.
+Works with arbitrary helm template flags like --values:
 
 ${CHART_NAME}: Name of helm chart.
 ${CHART_PATH}: Chart templates directory.