-
Notifications
You must be signed in to change notification settings - Fork 95
Creates platform specific config and main modules #1482
Creates platform specific config and main modules #1482
Conversation
This commit separates platform specific config and plugin serving code from the generic one. It will aid development of the Windows plugin. * Extracts unix sock service from main.go into main_linux.go. * Moves linux specific config from config.go into config_linux.go. * Renames config_test.go to config_linux_test.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shahzeb and I have already reviewed this change on the private branch. Please get at least one more review from others.
e30a8f9
to
8bf5c20
Compare
@shaominchen sure, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
package config | ||
|
||
const ( | ||
// Default paths - used in log init in main() and test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can be made file level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may not necessarily be true in the future i.e. it's only true for DefaultConfigPath
and DefaultLogPath
. We may add more linux specific config here, which may not relate to paths. What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Let's keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit
vmdk_plugin/main.go
Outdated
func fullSocketAddress(pluginName string) string { | ||
return filepath.Join(pluginSockDir, pluginName+".sock") | ||
// Server responds to HTTP requests from Docker. | ||
type Server interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - Server
is way too generic. It is better to name something like PluginRequestsServer
(or whatever you prefer, as long as it is more specific than Server
(the nit still applies if it's an old code :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 4fb083b.
[CI SKIP] Known issue doc.liping (#1475) * Add known issue in README.md. [SKIP CI] Adds Microsoft/go-winio vendor package (#1473) This commit adds the Microsoft/go-winio vendor package, which will be used for implementing the npipe based Docker plugin for Windows. Creates platform specific config and main modules (#1482) This commit separates platform specific config and plugin serving code from the generic one. It will aid development of the Windows plugin. * Extracts unix sock service from main.go into main_linux.go. * Moves linux specific config from config.go into config_linux.go. * Renames config_test.go to config_linux_test.go. Adding a test for delete/recreate default vm group & fix cleanup in vmdk_ops_test.py (#1381) Log cleanup for VMListener : VM object not found (#1489) Update CONTRIBUTING.md Refreshing test setup requirement and bring contributing.md to current. [SKIP CI] Addressing Sam's comment revamping contributing.md Addressing Mark's comment Update CONTRIBUTING.md
Description
This commit separates platform specific config and plugin serving code from the generic one. It will aid to the development of the Windows plugin.
main.go
intomain_linux.go
.config.go
intoconfig_linux.go
.config_test.go
toconfig_linux_test.go
.Test Results
The
test-all
target passes locally with the following output (tail'd):