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

Merged
merged 2 commits into from
Jun 4, 2024

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!)

@alco alco merged commit cbd652d into main Jun 4, 2024
7 checks passed
@alco alco deleted the alco/database-require-ssl branch June 4, 2024 09:32
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