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

fix(create): Supporting SSL enforced PostgreSQL databases #2905

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

tomh4
Copy link
Contributor

@tomh4 tomh4 commented Jun 16, 2024

Description

If a db has mandatory ssl when connecting the current @vendure/create fails.
Implemented SSL Option for PostgreSQL
Added SSL option to populate function
fixes #2904

Breaking changes

No

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

    If a db has mandatory ssl when connecting the current @vendure/create fails.
    Implemented SSL Option for PostgreSQL
    Added SSL option to populate function
Copy link

vercel bot commented Jun 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jun 19, 2024 10:00am

Copy link

netlify bot commented Jun 16, 2024

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit 043b6b9
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/666ef37f03fc3000081dde37

@michaelbromley
Copy link
Member

Thanks for this PR - it's a good addition. 2 things:

  1. Can you make the PR against the minor branch, since this is a new feature?
  2. I'm wondering whether the question might become a source of confusion when people don't know whether they need SSL or not. They might think "oh SSL is more secure, I'd better set it to true", and then the connection will fail. What do you think?

@tomh4
Copy link
Contributor Author

tomh4 commented Jun 17, 2024

Hello!

  1. Yes I will do that
  2. That might happen, but I already made 'no' the default. I might add a note to only use SSL if the db requires it and also add a more detailed explanation to the readme?

@michaelbromley
Copy link
Member

Yeah, the "false" default is good, and yes probably just a note as suggested along the lines of "(only enable if you database provider supports SSL)" will be enough.

A further enhancement (which you don't need to add to this PR, but can if you like), would be to detect SSL connection errors during bootstrap of the server, and display an instruction to try again with the SSL option turned on.

If a db has mandatory ssl when connecting the current @vendure/create fails.
Implemented SSL Option for PostgreSQL
Added SSL option to populate function
Implemented error handling for error 28000 interpreted as SSL error
@tomh4
Copy link
Contributor Author

tomh4 commented Jun 19, 2024

I have added the error handling and description, now i need to find out on how to change the branch towards which I want to PR against 😄

@tomh4 tomh4 changed the base branch from master to minor June 20, 2024 07:04
@tomh4
Copy link
Contributor Author

tomh4 commented Jun 20, 2024

@michaelbromley ah that easy, thanks 😄 I changed it, anything else I need to do?

@michaelbromley michaelbromley merged commit 65b4f3c into vendure-ecommerce:minor Jun 20, 2024
13 checks passed
@michaelbromley
Copy link
Member

Nope, we're all good! Thanks for the contribution. This will be made available in the next minor release (v2.3)

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.

PostgreSQL Databases with SSL enforced not supported by create and populate
2 participants