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

Can't set boolean environment variables in Helm chart #627

Open
3 tasks
timmc-edx opened this issue May 6, 2024 · 3 comments
Open
3 tasks

Can't set boolean environment variables in Helm chart #627

timmc-edx opened this issue May 6, 2024 · 3 comments
Labels
escalate-to-psre Create a PSRE ticket for this issue

Comments

@timmc-edx
Copy link
Member

timmc-edx commented May 6, 2024

We can't set boolean-looking environment variables for containers using the extraEnvs config provided by the django-ida Helm chart.

Setting a value to true or false results in a sync failure with an error message like unrecognized type: string following a large blob of config. Inspecting the config, we can see it includes something like {\"name\":\"DD_DJANGO_USE_LEGACY_RESOURCE_FORMAT\",\"value\":true}—the boolean true rather than the string "true". This happens even if the boolean was originally quoted as a string. Likely we're falling prey to Jinja-templated-YAML failing to escape the values it writes, inadvertently converting strings to bools.

Setting a (quoted) boolean directly in the env subtree of the django-ida Helm chart works fine, so likely what we need to do is stringify and quote the env var values in the loop where we apply extraEnvs.

A/C:

  • Boolean environment variables can be set in extraEnvs
    • Sort of fixed, but need to switch from quote to (probably) string -- quote is for shell scripts
  • Optional: Validation in place to prevent committing the wrong type, if the fix still allows for that
  • Optional: Error message is improved
    • Currently it says unrecognized type: string but this seems to be backwards; the message should be unrecognized type: bool
@timmc-edx timmc-edx converted this from a draft issue May 6, 2024
@robrap robrap added the escalate-to-psre Create a PSRE ticket for this issue label Jun 11, 2024
Copy link

@jristau1984 jristau1984 moved this to Backlog in Arch-BOM Jul 1, 2024
@jristau1984
Copy link

@timmc-edx your response in the linked GSRE ticket looks like this has been resolved. Can you confirm that this ticket can be closed?

@timmc-edx
Copy link
Member Author

It was fixed in https://github.com/edx/helm-charts/pull/150/files but I may want to re-fix it—the function they used was quote, which is appropriate for use in shell scripts rather than for YAML files. I think we want string instead. It's relatively low priority but should be a quick fix (now that I have documented how to test helm-charts changes locally!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
escalate-to-psre Create a PSRE ticket for this issue
Projects
Status: Backlog
Development

No branches or pull requests

3 participants