From a95018d6a1ae38c3ac06d04e0cb1e930609e708a Mon Sep 17 00:00:00 2001 From: Amogh Date: Sun, 29 Jan 2023 14:42:15 +0530 Subject: [PATCH 01/10] Helm deployment fails when postgresql.nameOverride is used --- chart/templates/_helpers.yaml | 11 +++++++++++ .../templates/secrets/metadata-connection-secret.yaml | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/chart/templates/_helpers.yaml b/chart/templates/_helpers.yaml index 074ecc7d4914c..faab09eac7de0 100644 --- a/chart/templates/_helpers.yaml +++ b/chart/templates/_helpers.yaml @@ -832,3 +832,14 @@ Where `.` is the global variables scope and `.Values.workers` the local variable {{- end }} {{- end }} {{- end }} + +{{/* +Helper to get the postgresql connection string conditionally +*/}} +{{- define "airflow.default.postgres.connection.string" -}} +{{- if .Values.postgresql.nameOverride }} +{{- printf "%s" .Values.postgresql.nameOverride }} +{{- else }} +{{- printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace }} +{{- end }} +{{- end }} diff --git a/chart/templates/secrets/metadata-connection-secret.yaml b/chart/templates/secrets/metadata-connection-secret.yaml index a4441c42bd4de..51a23e0998ad7 100644 --- a/chart/templates/secrets/metadata-connection-secret.yaml +++ b/chart/templates/secrets/metadata-connection-secret.yaml @@ -19,7 +19,7 @@ ## Airflow Metadata Secret ################################# {{- if not .Values.data.metadataSecretName }} -{{- $metadataHost := .Values.data.metadataConnection.host | default (printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace) }} +{{- $metadataHost := .Values.data.metadataConnection.host | default (include "airflow.default.postgres.connection.string" .) }} {{- $pgbouncerHost := (printf "%s-%s.%s" .Release.Name "pgbouncer" .Release.Namespace) }} {{- $host := ternary $pgbouncerHost $metadataHost .Values.pgbouncer.enabled }} {{- $port := ((ternary .Values.ports.pgbouncer .Values.data.metadataConnection.port .Values.pgbouncer.enabled) | toString) }} From 386bbfa322ce22594157d1e88c61fada7542ce13 Mon Sep 17 00:00:00 2001 From: Amogh Date: Sun, 29 Jan 2023 20:32:43 +0530 Subject: [PATCH 02/10] Adding basic helm chart tests --- tests/charts/test_basic_helm_chart.py | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index a4f5ff2d4b30d..ce39e08c51b35 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -508,3 +508,35 @@ def test_invalid_dags_access_mode(self): def test_namespace_names(self, namespace): """Test various namespace names to make sure they render correctly in templates""" render_chart(namespace=namespace) + + @pytest.mark.parametrize() + def test_postgres_connection_url_no_override(self): + # no nameoverride provided + assert "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" == \ + render_chart( + "my-release", + values={ + "postgresql": { + "enabled": True, + } + }, + show_only=[ + "templates/secrets/metadata-connection-secret.yaml" + ] + )["data"]["connection"] + + @pytest.mark.parametrize() + def test_postgres_connection_url_name_override(self): + assert "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" == \ + render_chart( + "my-release", + values={ + "postgresql": { + "enabled": True, + "nameOverride": "overrideName" + } + }, + show_only=[ + "templates/secrets/metadata-connection-secret.yaml" + ] + )["data"]["connection"] From 5290127abb2a75c4cc56fe961f96059592a161b8 Mon Sep 17 00:00:00 2001 From: Amogh Date: Sun, 29 Jan 2023 14:42:15 +0530 Subject: [PATCH 03/10] Helm deployment fails when postgresql.nameOverride is used --- chart/templates/_helpers.yaml | 11 +++++++++++ .../templates/secrets/metadata-connection-secret.yaml | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/chart/templates/_helpers.yaml b/chart/templates/_helpers.yaml index 074ecc7d4914c..faab09eac7de0 100644 --- a/chart/templates/_helpers.yaml +++ b/chart/templates/_helpers.yaml @@ -832,3 +832,14 @@ Where `.` is the global variables scope and `.Values.workers` the local variable {{- end }} {{- end }} {{- end }} + +{{/* +Helper to get the postgresql connection string conditionally +*/}} +{{- define "airflow.default.postgres.connection.string" -}} +{{- if .Values.postgresql.nameOverride }} +{{- printf "%s" .Values.postgresql.nameOverride }} +{{- else }} +{{- printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace }} +{{- end }} +{{- end }} diff --git a/chart/templates/secrets/metadata-connection-secret.yaml b/chart/templates/secrets/metadata-connection-secret.yaml index a4441c42bd4de..51a23e0998ad7 100644 --- a/chart/templates/secrets/metadata-connection-secret.yaml +++ b/chart/templates/secrets/metadata-connection-secret.yaml @@ -19,7 +19,7 @@ ## Airflow Metadata Secret ################################# {{- if not .Values.data.metadataSecretName }} -{{- $metadataHost := .Values.data.metadataConnection.host | default (printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace) }} +{{- $metadataHost := .Values.data.metadataConnection.host | default (include "airflow.default.postgres.connection.string" .) }} {{- $pgbouncerHost := (printf "%s-%s.%s" .Release.Name "pgbouncer" .Release.Namespace) }} {{- $host := ternary $pgbouncerHost $metadataHost .Values.pgbouncer.enabled }} {{- $port := ((ternary .Values.ports.pgbouncer .Values.data.metadataConnection.port .Values.pgbouncer.enabled) | toString) }} From 5c2c55f237cb33954e6c1c6cc81e11e2444a655d Mon Sep 17 00:00:00 2001 From: Amogh Date: Sun, 29 Jan 2023 20:32:43 +0530 Subject: [PATCH 04/10] Adding basic helm chart tests --- tests/charts/test_basic_helm_chart.py | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index a4f5ff2d4b30d..ce39e08c51b35 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -508,3 +508,35 @@ def test_invalid_dags_access_mode(self): def test_namespace_names(self, namespace): """Test various namespace names to make sure they render correctly in templates""" render_chart(namespace=namespace) + + @pytest.mark.parametrize() + def test_postgres_connection_url_no_override(self): + # no nameoverride provided + assert "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" == \ + render_chart( + "my-release", + values={ + "postgresql": { + "enabled": True, + } + }, + show_only=[ + "templates/secrets/metadata-connection-secret.yaml" + ] + )["data"]["connection"] + + @pytest.mark.parametrize() + def test_postgres_connection_url_name_override(self): + assert "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" == \ + render_chart( + "my-release", + values={ + "postgresql": { + "enabled": True, + "nameOverride": "overrideName" + } + }, + show_only=[ + "templates/secrets/metadata-connection-secret.yaml" + ] + )["data"]["connection"] From 6a1859d313f63ca702512e8695cc7ae23201a7ac Mon Sep 17 00:00:00 2001 From: Amogh Date: Mon, 30 Jan 2023 17:40:39 +0530 Subject: [PATCH 05/10] Fixing unit tests --- tests/charts/test_basic_helm_chart.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index ce39e08c51b35..9dbcf85ae4bd3 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -509,7 +509,6 @@ def test_namespace_names(self, namespace): """Test various namespace names to make sure they render correctly in templates""" render_chart(namespace=namespace) - @pytest.mark.parametrize() def test_postgres_connection_url_no_override(self): # no nameoverride provided assert "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" == \ @@ -525,7 +524,6 @@ def test_postgres_connection_url_no_override(self): ] )["data"]["connection"] - @pytest.mark.parametrize() def test_postgres_connection_url_name_override(self): assert "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" == \ render_chart( From c52d3b789021b3ea035962d15db5104fff503364 Mon Sep 17 00:00:00 2001 From: Amogh Date: Mon, 30 Jan 2023 23:55:08 +0530 Subject: [PATCH 06/10] Handling review comments --- chart/templates/_helpers.yaml | 10 ---------- .../templates/secrets/metadata-connection-secret.yaml | 3 ++- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/chart/templates/_helpers.yaml b/chart/templates/_helpers.yaml index faab09eac7de0..bf359930c2361 100644 --- a/chart/templates/_helpers.yaml +++ b/chart/templates/_helpers.yaml @@ -833,13 +833,3 @@ Where `.` is the global variables scope and `.Values.workers` the local variable {{- end }} {{- end }} -{{/* -Helper to get the postgresql connection string conditionally -*/}} -{{- define "airflow.default.postgres.connection.string" -}} -{{- if .Values.postgresql.nameOverride }} -{{- printf "%s" .Values.postgresql.nameOverride }} -{{- else }} -{{- printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace }} -{{- end }} -{{- end }} diff --git a/chart/templates/secrets/metadata-connection-secret.yaml b/chart/templates/secrets/metadata-connection-secret.yaml index 51a23e0998ad7..de7df1cd6c694 100644 --- a/chart/templates/secrets/metadata-connection-secret.yaml +++ b/chart/templates/secrets/metadata-connection-secret.yaml @@ -19,7 +19,8 @@ ## Airflow Metadata Secret ################################# {{- if not .Values.data.metadataSecretName }} -{{- $metadataHost := .Values.data.metadataConnection.host | default (include "airflow.default.postgres.connection.string" .) }} +{{- $defaultMetadataHost := .Values.postgresql.nameOverride | default (printf "%s-%s.%s" .Release.Name "postgresql" .Release.Namespace) }} +{{- $metadataHost := .Values.data.metadataConnection.host | default $defaultMetadataHost }} {{- $pgbouncerHost := (printf "%s-%s.%s" .Release.Name "pgbouncer" .Release.Namespace) }} {{- $host := ternary $pgbouncerHost $metadataHost .Values.pgbouncer.enabled }} {{- $port := ((ternary .Values.ports.pgbouncer .Values.data.metadataConnection.port .Values.pgbouncer.enabled) | toString) }} From 1fc85067824305d0a67a40f1be00c57fbc5d70fb Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 31 Jan 2023 10:08:50 +0530 Subject: [PATCH 07/10] Handling review comments --- tests/charts/test_basic_helm_chart.py | 46 +++++++++++---------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index d3bf56dd5b7aa..26b985decfe38 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -509,34 +509,26 @@ def test_namespace_names(self, namespace): """Test various namespace names to make sure they render correctly in templates""" render_chart(namespace=namespace) - def test_postgres_connection_url_no_override(self): # no nameoverride provided - assert "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" == \ - render_chart( - "my-release", - values={ - "postgresql": { - "enabled": True, - } - }, - show_only=[ - "templates/secrets/metadata-connection-secret.yaml" - ] - )["data"]["connection"] - + doc = render_chart( + "my-release", + show_only=["templates/secrets/metadata-connection-secret.yaml"], + )[0] + assert ( + "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" + == doc["data"]["connection"] + ) def test_postgres_connection_url_name_override(self): - assert "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" == \ - render_chart( - "my-release", - values={ - "postgresql": { - "enabled": True, - "nameOverride": "overrideName" - } - }, - show_only=[ - "templates/secrets/metadata-connection-secret.yaml" - ] - )["data"]["connection"] + # nameoverride provided + doc = render_chart( + "my-release", + show_only=["templates/secrets/metadata-connection-secret.yaml"], + values={"postgresql": {"nameOverride": "overrideName"}}, + )[0] + + assert ( + "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" + == doc["data"]["connection"] + ) From e0d604bab679f4fa5c4c62977f3934845ed4ef64 Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 31 Jan 2023 19:52:48 +0530 Subject: [PATCH 08/10] Fixing staticcheck issues --- chart/templates/_helpers.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/chart/templates/_helpers.yaml b/chart/templates/_helpers.yaml index bf359930c2361..074ecc7d4914c 100644 --- a/chart/templates/_helpers.yaml +++ b/chart/templates/_helpers.yaml @@ -832,4 +832,3 @@ Where `.` is the global variables scope and `.Values.workers` the local variable {{- end }} {{- end }} {{- end }} - From a908262f6650e63ff8d587c69a5406026746aa55 Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 31 Jan 2023 21:33:34 +0530 Subject: [PATCH 09/10] Fixing failing UT --- tests/charts/test_basic_helm_chart.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index 26b985decfe38..f00575f805125 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -16,6 +16,7 @@ # under the License. from __future__ import annotations +import base64 import warnings from subprocess import CalledProcessError from typing import Any @@ -517,7 +518,7 @@ def test_postgres_connection_url_no_override(self): )[0] assert ( "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" - == doc["data"]["connection"] + == base64.b64decode(doc["data"]["connection"]) ) def test_postgres_connection_url_name_override(self): @@ -530,5 +531,5 @@ def test_postgres_connection_url_name_override(self): assert ( "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" - == doc["data"]["connection"] + == base64.b64decode(doc["data"]["connection"]) ) From d6e7ef3640394e5b91ee1630143aafbf34f228e5 Mon Sep 17 00:00:00 2001 From: Amogh Date: Tue, 31 Jan 2023 23:39:52 +0530 Subject: [PATCH 10/10] Fixing failing UT --- tests/charts/test_basic_helm_chart.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/charts/test_basic_helm_chart.py b/tests/charts/test_basic_helm_chart.py index f00575f805125..171a012d4dead 100644 --- a/tests/charts/test_basic_helm_chart.py +++ b/tests/charts/test_basic_helm_chart.py @@ -518,7 +518,7 @@ def test_postgres_connection_url_no_override(self): )[0] assert ( "postgresql://postgres:postgres@my-release-postgresql.default:5432/postgres?sslmode=disable" - == base64.b64decode(doc["data"]["connection"]) + == base64.b64decode(doc["data"]["connection"]).decode("utf-8") ) def test_postgres_connection_url_name_override(self): @@ -531,5 +531,5 @@ def test_postgres_connection_url_name_override(self): assert ( "postgresql://postgres:postgres@overrideName:5432/postgres?sslmode=disable" - == base64.b64decode(doc["data"]["connection"]) + == base64.b64decode(doc["data"]["connection"]).decode("utf-8") )