[helm] Inconsistencies about the generated chart

aqeelat commented 6 months ago

Helm Chart Version

0.53.159

What step the error happened?

On deploy

Revelant information

There are a few issues/inconsistencies in how the helm charts are generated.

[ ] not all env variables are not quoted
[ ] webapp.url is misindented, making it webapp.ingress.url, which is failing the deploy Error: UPGRADE FAILED: template: airbyte/templates/env-configmap.yaml:15:21: executing "airbyte/templates/env-configmap.yaml" at <.Values.webapp.api.url>: nil pointer evaluating interface {}.url
[x] there is no documentation for the required logging bucket name. The config map has state-storage but the minio tamplate has a script for creating two buckets state-storage and airbyte-dev-logs
[ ] Even though we're using minio, the env variable for the storage backend is called s3 in some places and storage in other places
[x] We have to specify global.database.secretName and global.database.secretValue even though we're using .Values.externalDatabase.existingSecret because of the bootloader https://github.com/airbytehq/airbyte-platform/blob/v0.50.50/charts/airbyte-bootloader/templates/pod.yaml#L63
[ ] The app version was bumped from 0.50.49 to 0.50.50 without updating the chart version. Installing with helm --version 0.53.159 uses image version 0.50.49 even though I ran helm repo update
[ ] The statement # -- Log4j configuration file, if not using minio, update to "log4j2-no-minio.xml" does not indicate which file to use with external minio
[ ] workload launcher and api server are included in chart version 0.53.159 but they do not have non-dev published images.

I will add any other issues I encounter.

I can open a PR to fix some of these issues.

Relevant log output

No response

marcosmarxm commented 6 months ago

Documentation was updated describing how to setup using S3. There is a work being done to update and show how to do using GCS. https://docs.airbyte.com/deploying-airbyte/on-kubernetes-via-helm#external-airbyte-database

I'll keep the issue open for the team take a look related to the other issues reported.

aqeelat commented 6 months ago

@marcosmarxm regarding that, I think the point number 5 should be unchecked because it does not make sense to have to define the secret and key in multiple places. Also, the point of using helm is to not have to update k8s manifests by hand. The issue is trivial and we can just update the bootloader or any place that uses DATABASE_PASSWORD to use the externalDatabase.secretName if it exists.

aqeelat commented 6 months ago

I can open a PR if that would convince you

marcosmarxm commented 6 months ago

@aqeelat thanks but at the moment Deployment team is already working to refactor and improve this part.

airbytehq / airbyte