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

advancedTLS: Combine ClientOptions and ServerOptions to just Options #7202

Merged
merged 3 commits into from
May 6, 2024

Conversation

gtcooke94
Copy link
Contributor

ClientOptions and ServerOptions were basically identical minus a few fields. Duplicating so much code and documentation is messy. Given both have a single field each that are not relevant to the other, I think that we can just combine them and document that.

RELEASE NOTES: none

@gtcooke94 gtcooke94 added the Type: Internal Cleanup Refactors, etc label May 6, 2024
@gtcooke94 gtcooke94 added this to the 1.64 Release milestone May 6, 2024
@gtcooke94 gtcooke94 requested a review from erm-g May 6, 2024 19:36
@gtcooke94 gtcooke94 removed the request for review from erm-g May 6, 2024 19:37
@gtcooke94 gtcooke94 requested a review from erm-g May 6, 2024 19:39
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor things.

// Deprecated: use Options instead.
type ServerOptions = Options

// Options contains the fields a user can configure when settings up TLS clients
Copy link
Member

Choose a reason for hiding this comment

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

*setting up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -261,9 +213,11 @@ type ServerOptions struct {
// RootOptions is OPTIONAL on server side. This field only needs to be set if
// mutual authentication is required(RequireClientCert is true).
RootOptions RootCertificateOptions
// If the server want the client to send certificates.
// If the server want the client to send certificates. This value is only
Copy link
Member

Choose a reason for hiding this comment

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

want -> wants. Or "requires" probably is more precise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@gtcooke94 gtcooke94 merged commit 911d549 into grpc:master May 6, 2024
12 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants