client: disable deprecated NewClient() and NewEnvClient()#44019
client: disable deprecated NewClient() and NewEnvClient()#44019thaJeztah wants to merge 1 commit intomoby:masterfrom
Conversation
client/client_deprecated.go
Outdated
| func NewClient(host string, version string, client *http.Client, httpHeaders map[string]string) (*Client, error) { | ||
| return NewClientWithOpts(WithHost(host), WithVersion(version), WithHTTPClient(client), WithHTTPHeaders(httpHeaders)) | ||
| return nil, errors.New("deprecated: use 'client.NewClientWithOpts(WithHost(host), WithVersion(version), WithHTTPClient(client), WithHTTPHeaders(httpHeaders))'") | ||
| } |
There was a problem hiding this comment.
This error might be confusing when (accidentally) visible to end-users of downstream projects.
Can we just remove this function so that downstreams can detect the change on the compilation time?
There was a problem hiding this comment.
That's a good point; it won't be a compile error.
What I tried to achieve is to make it something that cannot be ignored by project using the old code, but giving them enough guidance to fix it. Perhaps as an alternative, we could;
- change the signature to be incompatible (causing a compile failure); removing all returns.
- change the function's documentation to provide the instructions
Something like:
// NewClient initializes a new API client for the given host and API version.
// It uses the given http client as transport.
// It also initializes the custom http headers to add to each request.
//
// It won't send any version information if the version number is empty. It is
// highly recommended that you set a version or your client may break if the
// server is upgraded.
//
// This function is deprecated, and is no longer functional.
//
// If you're using this function, replace it with the code below to get
// the equivalent (non-deprecated) code to initialize a client with a
// fixed API version:
//
// client.NewClientWithOpts(
// WithHost(host),
// WithVersion(version),
// WithHTTPClient(client),
// WithHTTPHeaders(httpHeaders),
// )
//
// We recommend to enable API version negotiation, so that the client
// automatically negotiate the API version to use when connecting with the
// daemon. To enable API version negotiation, use the WithAPIVersionNegotiation()
// option instead of WithVersion(version):
//
// client.NewClientWithOpts(
// WithHost(host),
// WithAPIVersionNegotiation(),
// WithHTTPClient(client),
// WithHTTPHeaders(httpHeaders),
// )
//
// Deprecated: use NewClientWithOpts
func NewClient(host string, version string, client *http.Client, httpHeaders map[string]string) {}
// NewEnvClient initializes a new API client based on environment variables.
// See FromEnv for a list of support environment variables.
//
// This function is deprecated, and is no longer functional.
//
// If you're using this function, replace it with the code below to get
// the equivalent (non-deprecated) code:
//
// client.NewClientWithOpts(FromEnv)
//
// We recommend to enable API version negotiation, so that the client
// automatically negotiate the API version to use when connecting with the
// daemon. To enable API version negotiation, add the WithAPIVersionNegotiation()
// option:
//
// client.NewClientWithOpts(FromEnv, WithAPIVersionNegotiation())
//
// Deprecated: use NewClientWithOpts
func NewEnvClient() {}There was a problem hiding this comment.
Maybe just comment out the code without breaking the prototype and add deprecation document there?
There was a problem hiding this comment.
Yeah, I thought that having the stub would allow people to more easily find it back (at least, my usual approach is to have my IDE link to the right file) and to still have docs at pkg.go.dev, but perhaps just grep could work of course
27ef95c to
1ccbb0f
Compare
The `NewClient()` and `NewEnvClient()` functions were deprecated in 18.03 in commit 772edd0, but they continued to be functional. Searching public repositories on GitHub shows that there's quite some projects that haven't updated their code to use the replacements. This patch updates both functions to become non-functional (to enforce a compilation failure), but extend the function's documentaiton with instructions on migrating to their non-deprecated alternatives. This should hopefully assist projects to migrate their code. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1ccbb0f to
4e78954
Compare
|
@AkihiroSuda updated with what I described above; let me know if you think this is a better approach. |
|
@AkihiroSuda @corhere @cpuguy83 PTAL (happy to get feedback on the approach; mostly wanting to make the replacement discoverable) |
corhere
left a comment
There was a problem hiding this comment.
This feels really heavy-handed, and for what? The maintenance burden is trivial so we're just punishing downstream users who have not yet inlined the bodies of the deprecated constructors into their projects. I vote we keep these deprecated functions around in all current and future releases of package github.com/docker/docker/client for compatibility and remove them in the next semver-major version (read: when the package gets a different import path once we finish the moby transition). I know, I've heard it all before that we don't follow semver and have a history of making breaking changes to client methods. Making breaking changes to individual method signatures is one thing, but the constructors are the entrypoints into the package! Nearly every module which imports the client will be calling one of the constructors. That is a lot of modules to break when they add some dependabot-happy dependency which transitively requires the bleeding-edge version of the client. We've been burned by Go module dependency hell many times already; where's the empathy for our own downstream users?
Downstream consumers might start to migrate away from the NewClient constructor in their own time if it was correctly marked as deprecated. Could we try fixing the docs first before resorting to forcing the change upon users by intentionally breaking their builds?
- The
Deprecated:text needs to be at the start of a paragraph for it to be semantically meaningful. As of today, pkg.go.dev correctly rendersNewEnvClientas deprecated, but notNewClient. Staticcheck and other linters are likely using the same detection logic, so downstream project maintainers who would do the right thing and fix deprecation warnings when notified by their tools are likely unaware that they are even referencing a deprecated function at all. - The package-level documentation recommends using
NewClient!
|
In addition to @corhere's comments, I think creating runtime errors for this is not ideal and it would be better to error out at compile time. |
The
NewClient()andNewEnvClient()functions were deprecated in 18.03 incommit 772edd0 (#36140), but they continued to be functional.
Searching public repositories on GitHub shows that there's quite some projects
that haven't updated their code to use the replacements.
This patch updates both functions to become non-functional (to enforce a compilation
failure), but extend the function's documentaiton with instructions on migrating to
their non-deprecated alternatives. This should hopefully assist projects to migrate
their code.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)