Skip to content

plugingetter: Avoid all caps for constant declarations#29564

Merged
vdemeester merged 1 commit intomoby:masterfrom
aaronlehmann:getter-types
Jan 10, 2017
Merged

plugingetter: Avoid all caps for constant declarations#29564
vdemeester merged 1 commit intomoby:masterfrom
aaronlehmann:getter-types

Conversation

@aaronlehmann
Copy link

Go style calls for mixed caps instead of all caps:
https://golang.org/doc/effective_go.html#mixed-caps

Change LOOKUP, ACQUIRE, and RELEASE to Lookup, Acquire, and Release.

I will rebase this after #29556 is merged.

Note that vendoring will be an issue because of cyclic dependencies between repositories. We should probably vendor a fork of libnetwork, and then upstream the change.

@tonistiigi
Copy link
Member

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸
@aaronlehmann needs a rebase and vendoring 👼

@aaronlehmann
Copy link
Author

Given the cyclic dependency issue, vendoring will have to be a two-stage effort. Are people okay with the original plan of vendoring a libnetwork fork in this PR, and then upstreaming the change once this is merged?

@vdemeester
Copy link
Member

@aaronlehmann I'm sad we have cyclic dependency issues (libnetwork and swarmkit at least right ?) and we might want to work to sort this out. But this should not block this PR so I'm fine with vendoring a libnetwork fork in that PR and have a follow-up. Could you update vendor.conf to make this ? 👼

@cpuguy83
Copy link
Member

cpuguy83 commented Jan 4, 2017

Might be better for libnetwork to just copy rather than deal with these cyclic issues, no?
I can't see this ever changing.

Go style calls for mixed caps instead of all caps:
https://golang.org/doc/effective_go.html#mixed-caps

Change LOOKUP, ACQUIRE, and RELEASE to Lookup, Acquire, and Release.

This vendors a fork of libnetwork for now, to deal with a cyclic
dependency issue. The change will be upstream to libnetwork once this is
merged.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@aaronlehmann
Copy link
Author

Rebased and updated to vendor a temporary libnetwork fork.

@stevvooe
Copy link
Contributor

LGTM

How is this used in libnetwork?

@mavenugo
Copy link
Contributor

mavenugo commented Jan 10, 2017

@stevvooe libnetwork ultimately calls the driver APIs for any CNM events such as add-network, create-endpoint, etc... so it uses the plugin API to get the remote client to make that API invocation.

@aaronlehmann @cpuguy83 I think it is better to keep the Get API free of Acquire, Release and Lookup primitives and make these flags as internal implementation of plugin package. And let the plugin users to call simple APIs to achieve the same functionality. Having Acquire or Release flags as part of Get api isnt very intuitive. Also having explicit functions will avoid the cyclic dep issue (which is a plus).

@stevvooe
Copy link
Contributor

@mavenugo That is pretty unfortunate.

Could we get these refactored so libnetwork just takes some interface and the reference counting sillyness is unexported?

Either way, let's merge this.

@vdemeester vdemeester merged commit 9c96768 into moby:master Jan 10, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 10, 2017
@vdemeester
Copy link
Member

@aaronlehmann I'll let you update and re-vendor 👼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants