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

Libnetwork remove unused args from driver-registry #42572

Closed

Conversation

thaJeztah
Copy link
Member

@@ -55,7 +55,7 @@ type IPAMNotifyFunc func(name string, driver ipamapi.Ipam, cap *ipamapi.Capabili
type DriverNotifyFunc func(name string, driver driverapi.Driver, capability driverapi.Capability) error

// New returns a new driver registry handle.
func New(lDs, gDs interface{}, dfn DriverNotifyFunc, ifn IPAMNotifyFunc, pg plugingetter.PluginGetter) (*DrvRegistry, error) {
func New(dfn DriverNotifyFunc, ifn IPAMNotifyFunc, pg plugingetter.PluginGetter) (*DrvRegistry, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, the ifn IPAMNotifyFunc is not used anywhere in our code currently either

@thaJeztah thaJeztah added area/networking kind/refactor PR's that refactor, or clean-up code status/2-code-review labels Jun 30, 2021
@thaJeztah thaJeztah marked this pull request as ready for review June 30, 2021 11:34
vendor.conf Outdated
@@ -141,7 +141,7 @@ github.com/klauspost/compress a3b7545c88eea469c2246bee0e6c
github.com/pelletier/go-toml 65ca8064882c8c308e5c804c5d5443d409e0738c # v1.8.1

# cluster
github.com/docker/swarmkit 2dcf70aafdc9ea55af3aaaeca440638cde0ecda6 # master
github.com/docker/swarmkit 137a181673b213a2bd81d2b032007139d69325a7 https://github.com/thaJeztah/swarmkit.git # master + https://github.com/docker/swarmkit/pull/3015
Copy link
Member Author

Choose a reason for hiding this comment

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

Temporarily using my fork because of circular dependency

@thaJeztah thaJeztah force-pushed the libnetwork_remove_unused_args branch 2 times, most recently from ee639f6 to 5e068d8 Compare August 7, 2021 16:35
vendor.conf Outdated
@@ -142,7 +142,7 @@ github.com/klauspost/compress a3b7545c88eea469c2246bee0e6c
github.com/pelletier/go-toml 65ca8064882c8c308e5c804c5d5443d409e0738c # v1.8.1

# cluster
github.com/docker/swarmkit 3629f50980f6c0dd5ccd7dbfa0956b57ea0cd78d # master
github.com/docker/swarmkit remove_driverregistry_arg https://github.com/thaJeztah/swarmkit.git # master + https://github.com/docker/swarmkit/pull/3015
Copy link
Member

Choose a reason for hiding this comment

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

👀

Copy link
Member Author

Choose a reason for hiding this comment

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

oh! yes, this one needs some coordination with @dperny to update moby/swarmkit#3015 once this is merged, and then revendor swarmkit from upstream after that.

Yay for circular dependencies

@thaJeztah thaJeztah marked this pull request as draft June 30, 2023 09:55
@thaJeztah thaJeztah force-pushed the libnetwork_remove_unused_args branch 2 times, most recently from e077c91 to ae92825 Compare July 4, 2023 10:42
@thaJeztah thaJeztah force-pushed the libnetwork_remove_unused_args branch from ae92825 to b040b08 Compare July 4, 2023 11:31
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

The migration path away from the deprecated drvregistry functions and methods with unused arguments is to migrate to the new drvregistry.Networks and drvregistry.IPAMs types. I was very deliberate in designing the replacements to force consumers to find another way to pass the PluginGetter when initializing the remote driver than by indirecting it through the driver registry. There is no good reason to modify the signatures of deprecated functions which are going away once Swarmkit migrates away from them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants