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(electric): Fix the order of case clauses when parsing database SSL config #1261

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

Conversation

alco
Copy link
Member

@alco alco commented May 14, 2024

@icehaunter spotted this in #1249 (comment).

I have reordered the case clauses by specificity and added comments for readability.

@icehaunter
Copy link
Contributor

This is a breaking change because it switches the default from false to true on DATABASE_REQUIRE_SSL. This matches the docs though. This is specified in the issue VAX-1839. So coordinate with @samwillis and @balegas on this

@alco
Copy link
Member Author

alco commented May 14, 2024

When the default value of DATABASE_REQUIRE_SSL was set to true in #767 (2023-12-22), it was implemented correctly. The later change that accidentally flipped the default (#848) was made on 2024-01-18.

I would argue this is a bug fix, more than a breaking change, but still worth mentioning in the Release Notes.

@alco alco force-pushed the alco/database-require-ssl branch from 9d43c9a to 3750552 Compare May 14, 2024 13:07
@alco
Copy link
Member Author

alco commented May 14, 2024

Corrections:

  • the original enforcement of SSL connections by default was released in v0.9.0 on 2024-01-18.
  • the "fix" that added support for sslmode and accidentally flipped the default value was released in v0.9.1 on 2024-01-24.

@samwillis
Copy link
Contributor

I agree it should be should to true.

@alco
Copy link
Member Author

alco commented May 24, 2024

I have prepared a draft of release notes if we release this change as part of a patch release.

@msfstef
Copy link
Contributor

msfstef commented May 30, 2024

Seems like everyone agrees on this change (I see Valter acked on Linear too) so I think we can get this merged? (I've had deploy issues/confusion as well because of this so making sure we get it merged!)

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.

None yet

4 participants