dmathieu
commented
1 year ago How about:
Introduce a new StaticConfig struct within go-elasticsearch. This new struct doesn't include any functions and interfaces.
Then, use functional options in elasticsearch.New to allow building a new client with static config (from YAML), but also add other options such as RetryOnError.
type StaticConfig struct {
Addresses []string `json:"addresses"`
}
myConfig := StaticConfig{}
client := elasticsearch.New(
WithStaticConfig(myConfig),
)With this, the user sets static config from the specified format. I am specifying the JSON key in this struct here, as it's the easiest way to avoid folks from having to set their own config. But more complicated solutions using reflect may be possible if we want to be fancy.
If they want to specify function configs, which can't be specified from anything static, they can use other functional options:
client := elasticsearch.New(
WithRetryOnError(func(*http.Request, error) bool {
return false
}),
)Within the New method, a Config struct would be built, and every functional option would be passed to it. Each option decides what to do with the config (set one or multiple attributes).
If a specific functional option isn't passed, a default value has to be set.
florianl
Status Quo
go-elasticsearch.Config defines how one can interact with Elasticsearch. At the moment this struct holds a mix of types, functions and interfaces.
Functions: https://github.com/elastic/go-elasticsearch/blob/624594e8d72aad852711469005959dbc8238f2f2/elasticsearch.go#L89 https://github.com/elastic/go-elasticsearch/blob/624594e8d72aad852711469005959dbc8238f2f2/elasticsearch.go#L103 https://github.com/elastic/go-elasticsearch/blob/624594e8d72aad852711469005959dbc8238f2f2/elasticsearch.go#L110
Interfaces: https://github.com/elastic/go-elasticsearch/blob/624594e8d72aad852711469005959dbc8238f2f2/elasticsearch.go#L106-L107
Problem
The functions and interfaces in go-elasticsearch.Config prevent users of elastic/go-elasticsearch to directly use go-elasticsearch.Config. The direct reuse of go-elasticsearch.Config is prevented, because it can not be configured, usually via
YAML, and services are required to allow users to configure the connection to Elasticsearch. Common dependencies to handle configuration within the Elastic ecosystem are elastic/go-ucfg and elastic/elastic-agent-libs/config. As it is not possible to configure interfaces and/or functions withYAML, services that have to use go-elasticsearch.Config come up with their reimplemenation of go-elasticsearch.Config:None of these reimplementations of go-elasticsearch.Config fully cover all parts of go-elasticsearch.Config , lead to duplicate code and might introduce subtile bugs. In addition, configuring the retry functionality is different from service to service and prevents the reuse of configurations.
Proposal
Remove interfaces and functions from go-elasticsearch.Config. One option to keep functionality is, is to introduce Getter/Setter.
The suggested proposal is a breaking change and not backwards compatible.