oauth2-proxy / manifests

For hosting manifests to allow for the deployment of OAuth2-Proxy/OAuth2-Proxy
Apache License 2.0
170 stars 153 forks source link

fix: allow literal 0 values in deployment.yaml extraArgs templating #139

Closed detvdl closed 1 year ago

detvdl commented 1 year ago

Some of the available arguments to be passed to extraArgs allow for literal 0 values, such as the --cookie-refresh setting.

However, setting it to 0 causes the {{ if $value }} check to logically return as falsy, thanks to gotpl.

This replacement check instead looks for invalid value kinds (i.e. nil/null, explicitly empty)

pierluigilenoci commented 1 year ago

@detvdl, could you please also add a chart version bump? And maybe a test?

detvdl commented 1 year ago

Hey @pierluigilenoci , sorry for the lack of response, totally lost track of this. Will take a look at adding a test later today.

pierluigilenoci commented 1 year ago

@detvdl, thank you!

detvdl commented 1 year ago

Hey @pierluigilenoci ,

Given the limitations/scope of chart-testing (CT), I don't see an ideal way to test whether literal 0 values are in fact passed through to the deployment correctly. I could just add something like

extraArgs:
  cookie-refresh: 0

To any of the test configurations, but that won't verify anything, as the problem was that values simply do not get passed due to the conditional, and that won't crash the container(s) nor get caught by linting

Which approach do you prefer for testing:

  1. Helm chart tests (https://helm.sh/docs/topics/chart_tests/) which invoke an actual container to verify whether the current template passes these value types
  2. Custom tests with e.g. TerraTest in Go, which purely act on the rendered templates
  3. Other?
pierluigilenoci commented 1 year ago

@detvdl, forget about it. In the end, I realized it wasn't worth it. I'm sorry if I wasted your time.

detlevvandaele-tomtom commented 1 year ago

No time wasted, no worries 🙂