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

api/server: drop unused Config struct #45083

Merged
merged 1 commit into from Mar 1, 2023

Conversation

corhere
Copy link
Contributor

@corhere corhere commented Feb 28, 2023

The Server.cfg field is never referenced by any code in package "./api/server". "./api/server".Config struct values are used by DaemonCli code, but only to pass around configuration copied out of the daemon config within the "./cmd/dockerd" package. Delete the "./api/server".Config struct definition and refactor the "./cmd/dockerd" package to pull configuration directly from cli.Config.

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

The Server.cfg field is never referenced by any code in package
"./api/server". "./api/server".Config struct values are used by
DaemonCli code, but only to pass around configuration copied out of the
daemon config within the "./cmd/dockerd" package. Delete the
"./api/server".Config struct definition and refactor the "./cmd/dockerd"
package to pull configuration directly from cli.Config.

Signed-off-by: Cory Snider <csnider@mirantis.com>
@corhere corhere added area/cli kind/refactor PR's that refactor, or clean-up code labels Feb 28, 2023
@@ -161,9 +161,7 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) {
}
}

cli.api = apiserver.New(serverConfig)
Copy link
Member

Choose a reason for hiding this comment

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

Where does the actual initialisation of the Server struct happen now? (i should probably check out the code locally 😂)

Also was scratching my head a bit why the "constructor" didn't actually construct everything, other than setting the "config" (but none of the other fields). Is that something we would "want" to be more of an actual constructor instead? Or not worth bothering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where does the actual initialisation of the Server struct happen now?

Initialization of the apiserver.Server struct happens in the cli.initMiddlewares and initRouter calls. That is true on master and it is true in this PR branch. The only meaningful change is that cli.api is no longer a pointer.

Also was scratching my head a bit why the "constructor" didn't actually construct everything, other than setting the "config" (but none of the other fields). Is that something we would "want" to be more of an actual constructor instead? Or not worth bothering?

I'm not convinced it's worth bothering. apiserver.Server implements the builder pattern, which seems to work well enough for this use case. I wish that its build method was named something more intuitive than Wait(), but that's out of scope for this PR.

@corhere corhere requested a review from neersighted March 1, 2023 01:19
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable simplification to me.

@corhere corhere merged commit 7568bbc into moby:master Mar 1, 2023
@corhere corhere deleted the unused-api-config-struct branch March 1, 2023 14:38
@thaJeztah thaJeztah added this to the v-next milestone Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli kind/refactor PR's that refactor, or clean-up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants