-
Notifications
You must be signed in to change notification settings - Fork 0
Official docker client #42
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
Conversation
6d29011 to
6fb9f3d
Compare
cmd/skpr/package/command.go
Outdated
|
|
||
| // See if we're using default builder. | ||
| userConfig, _ := user.NewClient() | ||
| featureFlags, _ := userConfig.LoadFeatureFlags() |
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.
We've already loaded the feature flags up in the main.go
The client feature flag could get passed down into this NewCommand.
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 did look at that... but found that every single function in userConfig does a .load(). That .load() isn't cached and reads the file every time. So I figured I wouldn't add in the complexity of passing it through all the way. But I'll update it.
internal/docker/factory.go
Outdated
|
|
||
| func NewClientFromUserConfig(auth auth.Auth) (DockerClient, error) { | ||
| // See if we're using default builder. | ||
| userConfig, _ := user.NewClient() |
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.
Errors aren't being handled.
I also think the name of the client should be passed in vs loaded multiple times. This set of packages doesn't have to be tightly coupled to the feature flags.
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.
Ignoring the errors was intentional. The config is only needed if it exists, if it has a docker-client setting and if that setting is a valid option... otherwise it uses the default. So if the config fails to load, we use the defaults.
But I guess it doesn't matter cause main.go throws error if it fails to load.
6fb9f3d to
6975b8a
Compare
Refactor
skpr packageandskpr pullto use a replaceable docker client. Implement an experimental config to switch between implementations. Set the default implementation to the existing go client.Add the following to your configuration to test it.