-
Notifications
You must be signed in to change notification settings - Fork 892
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
GODRIVER-3434 Revert builder-lister pattern for client options #1899
Conversation
API Change Report./v2/mongoincompatible changes##Connect: changed from func(..../v2/mongo/options.Lister[./v2/mongo/options.ClientOptions]) (Client, error) to func(..../v2/mongo/options.ClientOptions) (*Client, error) ./v2/mongo/optionsincompatible changes(*AutoEncryptionOptionsBuilder).List: removed compatible changes(*AutoEncryptionOptions).SetBypassAutoEncryption: added ./v2/x/mongo/driver/topologyincompatible changes##ConvertToDriverAPIOptions: changed from func(./v2/mongo/options.Lister[./v2/mongo/options.ServerAPIOptions]) ./v2/x/mongo/driver.ServerAPIOptions to func(./v2/mongo/options.ServerAPIOptions) ./v2/x/mongo/driver.ServerAPIOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
mongo/options/clientoptions.go
Outdated
|
||
return nil | ||
}) | ||
func (c *ClientOptions) SetAutoEncryptionOptions(aeopts Lister[AutoEncryptionOptions]) *ClientOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For AutoEncryptionOptions
, LoggerOptions
, and ServerAPIOptions
, should we also revert to the previous pattern (i.e. not use of Lister
type)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good other than some typos.
Co-authored-by: Qingyang Hu <[email protected]>
Co-authored-by: Qingyang Hu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
GODRIVER-3434
Summary
Don't apply the builder-lister pattern for client options.
Background & Motivation
This update has been a source of frustration for users migrating to the v2 beta. While the lister-builder pattern is effective for operational options, client options are often defined as struct literals and used in test / general comparison within a users code. For example:
While this isn't too difficult to manage in v2, it does require a code change. The primarily discussed alternative is to apply a
Merge
function toClientOptionsBuilder
. However, this would still require a code change and is kind of confusing for people who aren't familiar with the history of the problem in the Go Driver. This PR suggests deferring that question to v3, as client options are rather unique.