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

Add support for existing K8s secret for Database #107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lbanita
Copy link

@lbanita lbanita commented Jul 4, 2024

What does this PR change?

  • Allows referencing existing secrets to provide external database credential

Does the PR depend on any other PRs or Issues? If yes, please list them.

  • No

Checklist

I confirm, that I have...

Signed-off-by: Liviu Banita <liviulaurentiub@gmail.com>
@lbanita
Copy link
Author

lbanita commented Jul 10, 2024

Sorry, I am a bit confused at the error. As evidenced by following line ( and the Files changed), I did run helm-docs and helm-charts/ztka/README.MD has been updated

| deploy.postgresql.existingSecret | string | `""` | Postgresql existing Kubernetes secret for database authentication. Overwrites `deploy.postgresql.dsn`, `deploy.postgresql.address`, `deploy.postgresql.username`, `deploy.postgresql.password` and `deploy.postgresql.database` The Kubernetes secret must contain all values it overrides |

Autogenerated from chart metadata using [helm-docs v1.13.1](https://github.com/norwoodj/helm-docs/releases/v1.13.1)

Could someone clarify what I am missing in the process so I can do it correctly? Thank you.

@sl1pm4t
Copy link

sl1pm4t commented Jul 25, 2024

@lbanita - thanks for creating this PR. I'd love to see this get merged.

In regards to the helm-docs CI issue, I notice the GitHub action uses an older version of helm-docs.
https://github.com/paralus/helm-charts/blob/main/.github/workflows/ci.yaml#L18

You might want to try downgrade to v1.11.0 to match, and re-run the helm-docs command.

When I run the following command against your PR branch:

helm-docs --chart-search-root=charts

helm-docs removes the Autogenerated from chart metadata using line from the README.md, which is probably what is happening in CI too.

Signed-off-by: Liviu Banita <liviulaurentiub@gmail.com>
@lbanita
Copy link
Author

lbanita commented Jul 25, 2024

@sl1pm4t thanks for the clarification

The issue seems to come from inconsistency in the helm-docs Go version ( that the CI action uses ) and the one you can install via brew or asdf.
norwoodj/helm-docs#162 (comment)

Even if I use version 1.11.0 from brew or asdf, I still get the version footer line because this is what the template says:

{{ template "helm-docs.versionFooter" . }}

Apparently the Go version ignores the version footer line in the template.

Signed-off-by: Liviu Banita <liviulaurentiub@gmail.com>
@sl1pm4t
Copy link

sl1pm4t commented Jul 25, 2024

Oh that's interesting. I didn't know there were alternative helm-docs implementations.
Glad you were able to figure it out.

I'm not a maintainer on this project so can't help merge, but hope it is merged soon.

@nlamirault
Copy link
Contributor

any news on this feature ? tks

nlamirault added a commit to portefaix/portefaix-kubernetes that referenced this pull request Sep 3, 2024
See paralus/helm-charts#107

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Copy link
Contributor

@nirav-rafay nirav-rafay left a comment

Choose a reason for hiding this comment

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

lgtm

@nirav-rafay
Copy link
Contributor

nirav-rafay commented Oct 14, 2024

@lbanita thanks for your contribution, we will get this in for our next release.

@@ -103,6 +103,8 @@ Get DB Address.
{{- define "ztka.dbAddr" -}}
{{- if .Values.deploy.postgresql.enable -}}
{{.Release.Name}}-postgresql.{{.Release.Namespace}}.svc.cluster.local
{{- else if .Values.deploy.postgresql.existingSecret -}}
{{- printf "%s" (tpl .Values.deploy.postgresql.existingSecret $) -}}
Copy link
Member

Choose a reason for hiding this comment

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

This is returning existing secret name, but instead we are expecting db address.

Need to fetch db address from k8s secret.

@@ -114,6 +116,8 @@ Get DB Username.
{{- define "ztka.dbUser" -}}
{{- if .Values.deploy.postgresql.enable -}}
{{.Values.postgresql.auth.username}}
{{- else if .Values.deploy.postgresql.existingSecret -}}
{{- printf "%s" (tpl .Values.deploy.postgresql.existingSecret $) -}}
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -125,6 +129,8 @@ Get DB Password.
{{- define "ztka.dbPassword" -}}
{{- if .Values.deploy.postgresql.enable -}}
{{.Values.postgresql.auth.password}}
{{- else if .Values.deploy.postgresql.existingSecret -}}
{{- printf "%s" (tpl .Values.deploy.postgresql.existingSecret $) -}}
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -136,6 +142,8 @@ Get DB Name.
{{- define "ztka.dbName" -}}
{{- if .Values.deploy.postgresql.enable -}}
{{.Values.postgresql.auth.database}}
{{- else if .Values.deploy.postgresql.existingSecret -}}
{{- printf "%s" (tpl .Values.deploy.postgresql.existingSecret $) -}}
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -149,6 +157,8 @@ Get DSN
postgres://{{ .Values.postgresql.auth.username }}:{{ .Values.postgresql.auth.password }}@{{.Release.Name}}-postgresql.{{.Release.Namespace}}.svc.cluster.local:5432/{{ .Values.postgresql.auth.database }}?sslmode=disable
{{- else if .Values.deploy.postgresql.dsn -}}
{{ .Values.deploy.postgresql.dsn }}
{{- else if .Values.deploy.postgresql.existingSecret -}}
{{- printf "%s" (tpl .Values.deploy.postgresql.existingSecret $) -}}
Copy link
Member

Choose a reason for hiding this comment

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

same as above

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.

5 participants