search

trinodb / charts

Apache License 2.0
151 stars 173 forks source link

It's not immediately obvious how the `additional*Properties` are supposed to be set #166

Closed chgl closed 5 months ago

chgl commented 6 months ago

In the values.yaml the default value for setting additionalProperties is set as {}, suggesting that a dict object is expected: https://github.com/trinodb/charts/blob/main/charts/trino/values.yaml#L199-L207. However, in the templates this value is processed using range (https://github.com/trinodb/charts/blob/main/charts/trino/templates/configmap-coordinator.yaml#L62-L64) suggesting a list of values. In theory, using the same approach as when configuring the additionalCatalog (https://github.com/trinodb/charts/blob/533ace5fa38949254b20966604843b96d5955be4/charts/trino/templates/configmap-catalog.yaml#L22-L25) would work:

additionalConfigProperties:
    new-properties: |
      internal-communication.shared-secret=${ENV:TRINO_SHARED_SECRET}
      http-server.process-forwarded=true
    more-properties: |
      web-ui.authentication.type=oauth2

However, the template is missing correct indentation (nindent 4). So instead one has to set the properties as a list:

  additionalConfigProperties:
    - "internal-communication.shared-secret=${ENV:TRINO_SHARED_SECRET}"
    - "http-server.process-forwarded=true"
    - "web-ui.authentication.type=oauth2"

To make configuration more consistent I would suggest to either:

  1. change the type of additionalConfigProperties to string so it can be configured as follows: 
      additionalConfigProperties: |
    internal-communication.shared-secret=${ENV:TRINO_SHARED_SECRET}
    http-server.process-forwarded=true
    web-ui.authentication.type=oauth2
  2. Keep the type as-is, but fix the template to include nindent 4.

Personally, I'd prefer 1. since its more obvious how the string settings correspond to the trino properties. In 2. the nested new-properties, more-properties don't serve a purpose anyway.

nineinchnick commented 6 months ago

We need to keep them as arrays to preserve backward compatibility. I've opened #169 to fix the defaults and add proper documentation with examples.

We'd have to add extra logic to recognize when they're defined as strings. We're open to contributions, but if that's desired, let's have a separate issue and/or PR for this.