You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "MonkeyCanCode (via GitHub)" <gi...@apache.org> on 2025/10/14 03:35:11 UTC
[PR] Correct template rendering for authentication options [polaris]
MonkeyCanCode opened a new pull request, #2808:
URL: https://github.com/apache/polaris/pull/2808
The Helm chart installation was failing with a type error when processing authentication values. This was caused by the `tpl` function being incorrectly used on string literals instead of templates, leading to a `cannot retrieve Template.Basepath` error.
This commit resolves the issue by removing the unnecessary `tpl` calls from the `polaris.configVolumeAuthenticationOptions` helper template.
Sample error reference:
```
➜ polaris git:(main) ✗ helm upgrade --install --namespace polaris \
--values helm/polaris/ci/persistence-values.yaml \
polaris helm/polaris
Release "polaris" does not exist. Installing it now.
Error: template: polaris/templates/deployment.yaml:130:12: executing "polaris/templates/deployment.yaml" at <include "polaris.configVolume" .>: error calling include: template: polaris/templates/_helpers.tpl:179:10: executing "polaris.configVolume" at <include "polaris.configVolumeAuthenticationOptions" (list "" .Values.authentication)>: error calling include: template: polaris/templates/_helpers.tpl:351:29: executing "polaris.configVolumeAuthenticationOptions" at <.>: wrong type for value; expected chartutil.Values; got []interface {}
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Correct template rendering for authentication options [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode merged PR #2808:
URL: https://github.com/apache/polaris/pull/2808
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Correct template rendering for authentication options [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2808:
URL: https://github.com/apache/polaris/pull/2808#issuecomment-3423358892
Hi @MonkeyCanCode thanks for spotting this issue!
I don't think the root cause is the usage of `tpl`. The issue seems to be with the `.` parameter. In this context, it does not point the the global `.Values` object, that's why the template rendering is failing.
I think that a more correct approach would be to pass the global `.Values` as a parameter to this function, just like we do in `polaris.authenticationOptions`. Something like:
```
{{- define "polaris.configVolumeAuthenticationOptions" -}}
{{- $realm := index . 0 -}}
{{- $auth := index . 1 -}}
{{- $global := index . 2 -}}
...
- secret:
name: {{ tpl $secretName $global }}
```
The choice of templetizing every value exposed in the chart is a bit opinionated, but at the same time it allows for a lot of flexibility when defining values. For example, the secret name could be defined as:
```yaml
authentication.tokenBroker.secret.name: {{ .Release.Namespace }}-auth-secret
```
I don't mind having a broader discussion about whether it makes sense to templetize everything, but here, my feeling is that we are trying to fix a wrong template function, so imho we shouldn't remove the `tpl` function call, but instead fix its invocation.
WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Correct template rendering for authentication options [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #2808:
URL: https://github.com/apache/polaris/pull/2808#issuecomment-3402766549
I'll defer to @adutra for reviewing this change... maybe there was a use case for `tpl`, of which I'm not aware :thinking:
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Correct template rendering for authentication options [polaris]
Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #2808:
URL: https://github.com/apache/polaris/pull/2808#issuecomment-3424638257
> Hi @MonkeyCanCode thanks for spotting this issue!
>
> I don't think the root cause is the usage of `tpl`. The issue seems to be with the `.` parameter. In this context, it does not point to the global `.Values` object, that's why the template rendering is failing.
>
> I think that a more correct approach would be to pass the global `.Values` as a parameter to this function, just like we do in `polaris.authenticationOptions`. Something like:
>
> ```
> {{- define "polaris.configVolumeAuthenticationOptions" -}}
> {{- $realm := index . 0 -}}
> {{- $auth := index . 1 -}}
> {{- $global := index . 2 -}}
> ...
> - secret:
> name: {{ tpl $secretName $global }}
> ```
>
> The choice of templetizing every value exposed in the chart is a bit opinionated, but at the same time it allows for a lot of flexibility when defining values. For example, the secret name could be defined as:
>
> ```yaml
> authentication.tokenBroker.secret.name: {{ .Release.Namespace }}-auth-secret
> ```
>
> I don't mind having a broader discussion about whether it makes sense to templetize everything, but here, my feeling is that we are trying to fix a wrong template function, so imho we shouldn't remove the `tpl` function call, but instead fix its invocation.
>
> WDYT?
@adutra thanks for the review. I am fine with making everything templatize. Updated the PR with the fix. Please take another look when you have time.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org