-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
bugfix: add default host config if not set #4155
bugfix: add default host config if not set #4155
Conversation
Build succeeded.
|
ddd67f0
to
231714a
Compare
Build succeeded.
|
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
oops #4154
|
didn't reproduce the panic in my env. weird |
remotes/docker/config/hosts.go
Outdated
} | ||
} | ||
hosts[len(hosts)-1].path = "/v2" | ||
if len(hosts) == 0 { |
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.
The comment is not being addressed and I probably should have added a comment related to hosts[len(hosts)-1].host == ""
. A configuration may be intentionally blocking the namespace by returning 0, that is why I am differentiating between nil and 0. If there is a bug, then maybe we need to make sure it is nil or find a better way to handle that case. Additionally, the configuration may not be configuring the host, that is why it checks to see if the last element is not set. The configuration would only ever have the last host empty because using the Docker-compatible cert files only one entry is set (with no host), or using the config file with defaults but not setting a host override.
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.
OK. will change back with > 1
to >=1
.
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.
before this commit, the empty host doesn't work because the make([]hostConfig, 1)
has one element which never meet >1
condition. `
231714a
to
0f73216
Compare
Build succeeded.
|
0f73216
to
452a954
Compare
Build succeeded.
|
If there is not specific host config, like ctr does, the resolver will fail to get host path. And this patch is to add default host config if needs. And default config host config should have all caps for pull and push. Signed-off-by: Wei Fu <fuweid89@gmail.com>
Signed-off-by: Derek McGowan <derek@mcgstyle.net>
452a954
to
067aba7
Compare
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.
Unit test added, LGTM
Build succeeded.
|
If there is not specific host config, like ctr does, the resolver will
fail to get host path. And this patch is to add default host config if
needs.
And default config host config should have all caps for pull and push.
Signed-off-by: Wei Fu fuweid89@gmail.com