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

Allow appConfig to be provided as a string #115

Conversation

maxnitze
Copy link
Contributor

@maxnitze maxnitze commented Jun 9, 2023

Description of the Bug

Providing the value for backstage.appConfig as a string does technically work, but is not allowed by the schema.

Description of the change

Allow key backstage.appConfig to be of type object or string.

Existing or Associated Issue(s)

None

Additional Information

The template app-config-configmap.yaml uses the common.tplvalues.render helper function from the bitnami common chart.

app-config.yaml: |
{{- include "common.tplvalues.render" ( dict "value" .Values.backstage.appConfig "context" $) | nindent 4 }}

That helper function supports both, string-type and object-type values.

https://github.com/bitnami/charts/blob/35bc6b1d5d87dfc3875ed09a45bffd2bddc9ee6d/bitnami/common/templates/_tplvalues.tpl#L2-L13

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

@maxnitze maxnitze requested a review from a team as a code owner June 9, 2023 11:07
@maxnitze maxnitze force-pushed the bugfix/allow_appconfig_as_string_in_values_schema branch 2 times, most recently from fdc645c to b3c0df4 Compare June 9, 2023 11:15
@maxnitze maxnitze force-pushed the bugfix/allow_appconfig_as_string_in_values_schema branch from b3c0df4 to 506294a Compare June 9, 2023 11:16
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

Hi @maxnitze! Thank you for opening a PR! 🙂

Yeah, sure, why not... LGTM 👍

IMO I would still advise using object syntax because YAML is a terrible language and for example, indent levels are not obvious in multiline string values. But I'm all for letting users choose what path they want to go. 🙂

@tumido tumido merged commit b06aba9 into backstage:main Jun 9, 2023
@maxnitze maxnitze deleted the bugfix/allow_appconfig_as_string_in_values_schema branch June 9, 2023 11:26
@maxnitze
Copy link
Contributor Author

maxnitze commented Jun 9, 2023

Thanks for the fast approval. In theory I agree with your comment about object vs. string.

But in case of the common.tplvalues.render helper using the string-typed value is a little more powerful than the object-typed. You can use the full "power" of the templating engine. We, for example, define some variables at the top to be used multiple times, like the baseUrl. You could also in- or exclude whole blocks of your configuration based on other values.

Also it was possible in the old version of the chart (we just update from 0.17.1) to use this and it is not anymore :)

Anyways. Thanks for taking care!

@tumido
Copy link
Member

tumido commented Jun 9, 2023

Oh... I totally didn't think of that @maxnitze! Yup, that makes great sense actually. Thank you! 🙂

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.

2 participants