Skip to content

Conversation

tkrajacic
Copy link
Contributor

When connecting through an established channel (e.g. an SSH tunnel) it can still be useful to allow postgres to connect using TLS. This PR exposes the tls parameter on the connection method for using an established channel.

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.78%. Comparing base (96ed89f) to head (539db2e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@ ## main #526 +/- ## ======================================= Coverage 61.77% 61.78% ======================================= Files 125 125 Lines 10072 10074 +2 ======================================= + Hits 6222 6224 +2  Misses 3850 3850 
Files with missing lines Coverage Δ
.../Connection/PostgresConnection+Configuration.swift 51.35% <100.00%> (+1.35%) ⬆️
Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Great addition! Thanks for opening a PR. Please see the comment! If you need help please reach out.

/// - tls: The TLS mode to use. Defaults to ``TLS-swift.struct/disable``.
public init(establishedChannel channel: Channel, username: String, password: String?, database: String?) {
self.init(endpointInfo: .configureChannel(channel), tls: .disable, username: username, password: password, database: database)
public init(establishedChannel channel: Channel, tls: PostgresConnection.Configuration.TLS = .disable, username: String, password: String?, database: String?) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking API change. Would you mind adding an additional initializer? This initializer should then call the new one that you just created.

Do you feel comfortable to add a unit test using a NIOAsyncTestingChannel? An example can be found here:
https://github.com/vapor/postgres-nio/blob/f2a6394a2e7157d547727b975fc0328b92f89fb1/Tests/PostgresNIOTests/New/PostgresConnectionTests.swift#L41C1-L82C1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably won't have time to do the test right now, as I need to fix the wrong channel handler order first for TLS 🤭

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to fix the wrong channel handler order first for TLS

Not sure I follow here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #527

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

In an ideal case we would add at least one unit test for this...

@fabianfett fabianfett added the semver-minor Adds new public API. label Dec 9, 2024
@fabianfett fabianfett merged commit fd0e415 into vapor:main Dec 10, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Adds new public API.

2 participants