Skip to content

Better connection strings #152

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

Merged
merged 17 commits into from
Apr 7, 2025

Conversation

egabancho
Copy link
Member

No description provided.

@plesoun-stein
Copy link
Contributor

we have cloudnative postgres operator and all params are in secret.
It should be nice to specify password from secret

helper should look like

{{- define "invenio.postgresql.password" -}}
  {{- if .Values.postgresql.enabled -}}
    {{- required "Missing .Values.postgresql.auth.password" .Values.postgresql.auth.password -}}
  {{- else if and ( not .Values.postgresql.enabled ) ( not ( empty .Values.postgresqlExternal.password )) -}}
    {{- .Values.postgresqlExternal.password -}}
  {{- else if and ( not .Values.postgresql.enabled ) ( empty .Values.postgresqlExternal.password ) ( not ( empty  .Values.postgresqlExternal.existingSecret )) }}
    {{- $obj := (lookup "v1" "Secret" $root.Release.Namespace .Values.postgresqlExternal.existingSecret ).data }}
    {{- if or ( empty $obj ) ( empty ( get $obj .Values.postgresqlExternal.existingSecretPasswordKey )) -}}
      {{- fail ( printf "\n\n\tthere is missing secret \`%s\` or password key \`%s\`" .Values.postgresqlExternal.existingSecret .Values.postgresqlExternal.existingSecretPasswordKey ) }}
    {{- end -}}
    {{- printf "%s" ( get $obj ( tpl .Values.postgresqlExternal.existingSecretPasswordKey . ) | b64dec )  }}
  {{- else -}}
    {{- fail ( printf "\n\n\tInternal postgresql chart is disabled and there is missing .Values.postgresqlExternal.existingSecret declaration or another mistake or typo\n\n%s" ( toYaml .Values.postgresqlExternal | indent 6 )) }}
  {{- end -}}
{{- end -}}

(it's not tested, only example as POC)
simmilar work may be done for other params. hostname, username etc.
and I cat write it reusable for rabbit and redis

I have a time and I can do it.

@egabancho egabancho force-pushed the better-connection-strings branch from 48995df to cf9e856 Compare March 3, 2025 10:19
@egabancho egabancho marked this pull request as ready for review March 14, 2025 21:21
@egabancho
Copy link
Member Author

@lindhe, Take another look 😉

@egabancho egabancho requested a review from lindhe March 14, 2025 21:22
@lindhe lindhe force-pushed the better-connection-strings branch from c7ac4a6 to 65f8d0c Compare March 18, 2025 10:25
@lindhe
Copy link
Contributor

lindhe commented Mar 18, 2025

I think maybe my merge via GitHub went wrong, because something is badly broken right now. I've reset to your last commit. Checking things again now…

Copy link
Contributor

@lindhe lindhe left a comment

Choose a reason for hiding this comment

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

There's a bunch of relatively minor comments here. But in my last test, I think the values looked pretty broken when I ran env in the container. I'll see if I can narrow down that issue while we're addressing my other comments here.

Samk13
Samk13 previously approved these changes Mar 19, 2025
@lindhe
Copy link
Contributor

lindhe commented Mar 19, 2025

It seems to work well, but I'm a bit confused with these variables:

$ kubectl exec deploy/invenio-web -c web -- env |  grep RABBITMQ_PORT
INVENIO_RABBITMQ_PORT_25672_TCP=tcp://10.43.72.254:25672
INVENIO_RABBITMQ_PORT_4369_TCP_ADDR=10.43.72.254
INVENIO_RABBITMQ_PORT_25672_TCP_ADDR=10.43.72.254
INVENIO_RABBITMQ_PORT_15672_TCP_ADDR=10.43.72.254
INVENIO_RABBITMQ_PORT_15672_TCP_PROTO=tcp
INVENIO_RABBITMQ_PORT=tcp://10.43.72.254:5672
INVENIO_RABBITMQ_PORT_25672_TCP_PORT=25672
INVENIO_RABBITMQ_PORT_5672_TCP_ADDR=10.43.72.254
INVENIO_RABBITMQ_PORT_25672_TCP_PROTO=tcp
INVENIO_RABBITMQ_PORT_5672_TCP_PROTO=tcp
INVENIO_RABBITMQ_PORT_4369_TCP_PORT=4369
INVENIO_RABBITMQ_PORT_5672_TCP_PORT=5672
INVENIO_RABBITMQ_PORT_4369_TCP_PROTO=tcp
INVENIO_RABBITMQ_PORT_4369_TCP=tcp://10.43.72.254:4369
INVENIO_RABBITMQ_PORT_15672_TCP=tcp://10.43.72.254:15672
INVENIO_RABBITMQ_PORT_15672_TCP_PORT=15672
INVENIO_RABBITMQ_PORT_5672_TCP=tcp://10.43.72.254:5672

I guess they are just strangely named variables in the container, not something we've set here that gets messed up, right?

@lindhe lindhe force-pushed the better-connection-strings branch from a0153bc to 400c50c Compare March 19, 2025 12:14
@egabancho
Copy link
Member Author

I don't see any of these variables in any of the containers I have (using my old branch with these changes). But, judging from the name, they might be some k8s thingy?

Copy link
Contributor

@lindhe lindhe left a comment

Choose a reason for hiding this comment

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

If you are happy with the changes now, I think this is ready to merge. 🙂

@carlinmack carlinmack moved this to Triage in v13.0.0 Mar 26, 2025
@max-moser max-moser moved this from Triage to To release in v13.0.0 Mar 29, 2025
@max-moser
Copy link

i'm not familiar enough to make a call with helm charts, but it looks like there was a discussion and it got accepted – so i moved it to "to release"

@lindhe
Copy link
Contributor

lindhe commented Mar 29, 2025

@max-moser I still have some unresolved comments. No release yet.

@utnapischtim utnapischtim moved this from To release to 🏗 In progress in v13.0.0 Apr 1, 2025
@egabancho
Copy link
Member Author

I can't approve the PR because I made it 😛
@lindhe, if you're happy with it, I am happy with it.

@lindhe
Copy link
Contributor

lindhe commented Apr 4, 2025

I can't approve the PR because I made it 😛 @lindhe, if you're happy with it, I am happy with it.

Thanks. I'll add the last change with comments that I mentioned, then I'll approve. Then you merge if you are happy with my changes. 😛

@lindhe
Copy link
Contributor

lindhe commented Apr 7, 2025

Turns out I already made that change, totally forgot about that! 😄 I think this looks good now. I'll just one quick test to see that I didn't miss something obvious, then I'll approve.

@egabancho egabancho merged commit 7aea719 into inveniosoftware:master Apr 7, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to To release 🤖 in v13.0.0 Apr 7, 2025
@egabancho egabancho deleted the better-connection-strings branch April 7, 2025 13:11
@lindhe lindhe linked an issue Apr 8, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To release 🤖
Development

Successfully merging this pull request may close these issues.

Granular env-based solution for "connection string"-like config
5 participants