-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remove docker api dependency from cli/config #1642
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
|
cc @tonistiigi |
9d07350 to
0d8485a
Compare
0d8485a to
2f04b24
Compare
Codecov Report
@@ Coverage Diff @@
## master #1642 +/- ##
==========================================
+ Coverage 55.69% 55.69% +<.01%
==========================================
Files 301 301
Lines 20470 20484 +14
==========================================
+ Hits 11400 11409 +9
- Misses 8250 8252 +2
- Partials 820 823 +3 |
Codecov Report
@@ Coverage Diff @@
## master #1642 +/- ##
==========================================
+ Coverage 56.11% 56.11% +<.01%
==========================================
Files 306 306
Lines 20895 20909 +14
==========================================
+ Hits 11725 11734 +9
- Misses 8326 8328 +2
- Partials 844 847 +3 |
| // ParseProxyConfig computes proxy configuration by retrieving the config for the provided host and | ||
| // then checking this against any environment variables provided to the container | ||
| func (configFile *ConfigFile) ParseProxyConfig(host string, runOpts []string) map[string]*string { | ||
| func (configFile *ConfigFile) ParseProxyConfig(host string, runOpts map[string]*string) map[string]*string { |
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.
Ah, funny, I was working on some refactoring for this a while back related to #1574 (I see I have two implementations, but trying to recall now what was the main difference; 24e1f96 and f0f3511 (master...thaJeztah:refactor_proxy_config and master...thaJeztah:refactor_proxy_config2)
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.
oh, related to that; perhaps you could review #1573 😇
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 don't think they are equivalent refactors. I'm trying to remove dependency of docker/docker from cli/config. It's not pretty but this is what I have :)
vdemeester
left a comment
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 💇♂️
|
@thaJeztah mergeable? |
silvin-lubecki
left a comment
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!
thaJeztah
left a comment
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.
left two comments, but otherwise looks good
Signed-off-by: Tonis Tiigi <[email protected]> Signed-off-by: Tibor Vass <[email protected]>
2f04b24 to
27b2797
Compare
thaJeztah
left a comment
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, thanks!
Signed-off-by: Tonis Tiigi [email protected]
Signed-off-by: Tibor Vass [email protected]