Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the vault lookup template env value #116

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

gowrisankar22
Copy link
Contributor

Existing Issue

concourse/concourse#5738

Why do we need this PR?

Fix the lookup template example.

Changes proposed in this pull request

  • Fix the lookup template example.

Contributor Checklist

Are the following items included as part of this PR? Please delete checkbox items that don't apply.

  • Variables are documented in the README.md

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not
fill out this section.

  • Code reviewed
  • Topgun tests run
  • Back-port if needed

Signed-off-by: gowrisankar <gowrisankar.m01@sap.com>
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I think we can improve on this though and leave the original example as-is. It makes sense to have this variable still be a list of strings instead of a single string that happens to represent a list. In the end we do need a string but a list of strings is easier to manage/reason about and is a more accurate representation of this variable's type.

We can get the desired result by doing a join before quote'ing. Using quote alone results in a string representation of the list which is why the default example breaks. By adding join we get to define the separator and create the string in the correct format.

diff --git a/templates/web-deployment.yaml b/templates/web-deployment.yaml
index d75ad70..0afea90 100644
--- a/templates/web-deployment.yaml
+++ b/templates/web-deployment.yaml
@@ -507,7 +507,7 @@ spec:
             {{- end }}
             {{- if.Values.concourse.web.vault.lookupTemplates }}
             - name: CONCOURSE_VAULT_LOOKUP_TEMPLATES
-              value: {{ .Values.concourse.web.vault.lookupTemplates | quote }}
+              value: {{ .Values.concourse.web.vault.lookupTemplates | join "," | quote }}
             {{- end }}
             - name: CONCOURSE_VAULT_AUTH_BACKEND
               value: {{ .Values.concourse.web.vault.authBackend | quote }}

With the above change the web-deployment template renders a comma separated string instead of a string representation of a list:

            - name: CONCOURSE_VAULT_LOOKUP_TEMPLATES
              value: "/{{.Team}}/{{.Pipeline}}/{{.Secret}},/{{.Team}}/{{.Secret}}"

vs the original:

            - name: CONCOURSE_VAULT_LOOKUP_TEMPLATES
              value: "[/{{.Team}}/{{.Pipeline}}/{{.Secret}} /{{.Team}}/{{.Secret}}]"

wdyt?

Signed-off-by: gowrisankar <gowrisankar.m01@sap.com>
@gowrisankar22 gowrisankar22 changed the title Fix the vault lookup example Fix the vault lookup template env value Jun 18, 2020
@gowrisankar22
Copy link
Contributor Author

@taylorsilva it makes sense. I bumped the change. Please check

templates/web-deployment.yaml Outdated Show resolved Hide resolved
Co-authored-by: Taylor Silva <tsilva@pivotal.io>
Signed-off-by: gowrisankar <gowrisankar.m01@sap.com>
@taylorsilva taylorsilva merged commit 9a6c88e into concourse:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants