search

gadsme / charts

Gadsme Helm chart repository
Apache License 2.0
44 stars 25 forks source link

[cubestore] add Annotations and Environment Variables #18

Closed asproul closed 1 year ago

asproul commented 1 year ago
lvauvillier commented 1 year ago

Hi @asproul thanks for your PR.

Can you elaborate more on you use case? Is there some environment variables that are missing in the chart?

You can take a look to the cube chart. There is extraEnvVars, extraEnvVarsFromConfigMap and extraEnvVarsFromSecret options. Maybe we can keep the same terminology for consistency. What do you think?

asproul commented 1 year ago

Sorry for the delay in my response.

Can you elaborate more on you use case?

I am running cube as a GCP cloud run service which connects to cubestore running on a GKE autopilot cluster. Cube talks to the cubestore router using an internal load balancer over a private connection which is attached to the cubestore router kubernetes service. The load balancer is created using annotations and updating the spec for the router service (gke guide for an ilb).

My values.yaml has something similar to the following

router:
  service:
    annotations:
      networking.gke.io/load-balancer-type: "Internal"

and I set the type and traffic policy with

    spec:
      type: "LoadBalancer"
      externalTrafficPolicy: "Cluster"

Is there some environment variables that are missing in the chart?

My cubestore router running on a GKE autopilot cluster was logging multiple Capacity(MessageTooLong) errors and I had to set 2 environment variables(CUBESTORE_TRANSPORT_MAX_MESSAGE_SIZE and CUBESTORE_TRANSPORT_MAX_FRAME_SIZE) according to https://github.com/cube-js/cube/issues/6346. Instead of asking for these environment variables within the chart (which aren't listed on the cubejs environment variables page), I thought setting environment variables with a map in values.yaml seemed to add flexibility to the chart and future proof when more environment variables are added to cubestore.

What do you think?

Sounds good. I will rebase and update to use extraEnvVars, extraEnvVarsFromConfigMap, and extraEnvVarsFromSecret

lvauvillier commented 1 year ago

Merged, thanks @asproul