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.

Sign up for GitHub

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

Jump to bottom

Add ability to set database connection string #221

Closed
wants to merge 1 commit into from
Closed

Add ability to set database connection string #221

wants to merge 1 commit into from

Conversation

pbolduc
Copy link
Contributor

@pbolduc pbolduc commented Oct 12, 2023

Description

Adds the ability to use an optional onnectionString property. This would allow customizing any of the connecting string properties without having to configure each property individually.

Fixes #220

Types of changes

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@pbolduc
Copy link
Contributor Author

pbolduc commented Oct 12, 2023

I understand your team is currently in the process of a major 0.7 release. I would appreciate once the dust settles, if you could discuss and review this PR. I understand there may not be an appetite for exposing the connection string as a configuration variable directly.

@pbolduc
Copy link
Contributor Author

pbolduc commented Oct 12, 2023

For reference, I believe the Postgres client uses node-postgres which in turn uses pg-connection-string for parsing connection strings. The pg-connection-string package provides docs on the options for this field.

jujaga
jujaga suggested changes Oct 12, 2023
View reviewed changes
Copy link
Member

@jujaga jujaga left a comment

Choose a reason for hiding this comment

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

Applying a temporary review block on this as this addition will also require an appropriate change to the helm chart to properly support mounting connectionString into the deploymentConfig and preserving optionality behavior.

@TimCsaky
Copy link
Contributor

Closing this because it's been a few months.
We have captured this task in our backlog as a feature we may want to implement at a later date.

@TimCsaky TimCsaky closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jujaga jujaga jujaga requested changes

@norrisng-bc norrisng-bc Awaiting requested review from norrisng-bc norrisng-bc is a code owner

@TimCsaky TimCsaky Awaiting requested review from TimCsaky TimCsaky is a code owner

@jatindersingh93 jatindersingh93 Awaiting requested review from jatindersingh93 jatindersingh93 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow configure database connection ssl settings
3 participants
@pbolduc @TimCsaky @jujaga