Rationalise TLSConfiguration construction. #299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weissi
reviewed
May 13, 2021
| /// contexts, you should use `forServer` instead. | ||
| /// | ||
| /// For customising fields, modify the returned TLSConfiguration object. | ||
| public static func forClient() -> TLSConfiguration { |
There was a problem hiding this comment.
I think I'm okay with .forClient/Server but we should consciously decide that we're not sticking to the "Begin names of factory methods with “make”, e.g. x.makeIterator()." Swift naming guideline here.
Other options could be:
(TLSConfiguration).makeClientTLSConfiguration()(TLSConfiguration).makeClientConfiguration()
Sadly, we can't really go for inits here because the client constructor doesn't have arguments so there's no where to stick the word "client".
glbrntt
reviewed
May 13, 2021
| /// Create a TLS configuration for use with server-side contexts. This allows setting the `NIOTLSCipher` property specifically. | ||
| /// | ||
| /// This provides sensible defaults while requiring that you provide any data that is necessary | ||
| /// for server-side function. For client use, try `forClient` instead. | ||
| @available(*, deprecated, renamed: "TLSConfiguration.forServer(certificateChain:privateKey:)") |
There was a problem hiding this comment.
Is this right (and elsewhere)? I think we want just "forServer(certificateChain:privateKey:)" here otherwise Xcode shoves in the TLSConfiguration as well when you apply the fix its.
agnosticdev
reviewed
May 13, 2021
Lukasa
force-pushed
the
cb-new-config-init
branch
from
7e4b112
to
5e7e80b
Compare
Lukasa
force-pushed
the
cb-new-config-init
branch
2 times, most recently
from
865fbf3
to
3ed9d64
Compare
glbrntt
approved these changes
May 13, 2021
Lukasa
force-pushed
the
cb-new-config-init
branch
from
3ed9d64
to
9fb90e5
Compare
Motivation: Currently whenever we add new config to TLSConfiguration, we add a new static factory function that needs to provide the field. This is because we currently allow configuration primarily by way of initializers. This is increasingly not scaling, leading to a proliferation of almost identical static factory functions. We can replace this proliferation by having only "default" constructions that default basically everything, and then having users use getters/setters to initialize things. This also works well when we have multiple ways of interacting with config, e.g. with the various cipher suite operations. Modifications: - Deprecate all existing factory functions. - Replace with two: forClient() and forServer(certificateChain:privateKey:). These configure only the mandatory things for each mode. - Replace all uses of the deprecated initializers with the new ones, configuring fields manually as needed. Result: More consistent configuration objects.
Lukasa
force-pushed
the
cb-new-config-init
branch
from
9fb90e5
to
53d0e28
Compare
fabianfett
reviewed
May 13, 2021
fabianfett
approved these changes
May 13, 2021
weissi
approved these changes
May 13, 2021
Davidde94
approved these changes
May 13, 2021
|
@swift-nio-bot test this please |
Co-authored-by: Fabian Fett <fabianfett@apple.com>
|
@swift-nio-bot test this please |
2 tasks
mackoj
added a commit
to mackoj/APNSwift
that referenced
this pull request
Sep 16, 2021
Since you use `.makeClientConfiguration()` and it has been introduce in PR apple/swift-nio-ssl#299 please update your base requirement of `swift-nio-ssl`. Please consider release a point update. Thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Currently whenever we add new config to TLSConfiguration, we add a new
static factory function that needs to provide the field. This is because
we currently allow configuration primarily by way of initializers. This
is increasingly not scaling, leading to a proliferation of almost
identical static factory functions.
We can replace this proliferation by having only "default" constructions
that default basically everything, and then having users use
getters/setters to initialize things. This also works well when we have
multiple ways of interacting with config, e.g. with the various cipher
suite operations.
Modifications:
forServer(certificateChain:privateKey:). These configure only the
mandatory things for each mode.
configuring fields manually as needed.
Result:
More consistent configuration objects.