Skip to content
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

feature: add default registry namespace #911

Merged
merged 1 commit into from
Mar 21, 2018
Merged

feature: add default registry namespace #911

merged 1 commit into from
Mar 21, 2018

Conversation

Ace-Tang
Copy link
Contributor

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

add registry namespace in config, it will be used in pull/push/search images.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Mar 16, 2018

Codecov Report

Merging #911 into master will increase coverage by <.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
+ Coverage   13.66%   13.67%   +<.01%     
==========================================
  Files         121      121              
  Lines        7667     7666       -1     
==========================================
  Hits         1048     1048              
+ Misses       6529     6528       -1     
  Partials       90       90
Impacted Files Coverage Δ
pkg/reference/def.go 33.33% <ø> (ø) ⬆️
daemon/mgr/image.go 36.26% <0%> (+0.39%) ⬆️
daemon/mgr/image_utils.go 0% <0%> (ø) ⬆️
pkg/reference/parse.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa50191...1c9822c. Read the comment docs.

@HusterWan
Copy link
Contributor

LGTM

@@ -19,7 +20,7 @@ func (mgr *ImageManager) addRegistry(input string) string {
if _, ok := reference.Domain(input); ok {
return input
}
return mgr.DefaultRegistry + input
return filepath.Join(mgr.DefaultRegistry, mgr.DefaultNamespace, input)
Copy link
Contributor

Choose a reason for hiding this comment

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

That means we only specify the image name after merge this pr, just like pouch pull centos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it just follow original logic, not change it, what I do is to split docker.io/library into docker.io + library.

@@ -81,7 +81,8 @@ func setupFlags(cmd *cobra.Command) {
flagSet.BoolVar(&cfg.IsLxcfsEnabled, "enable-lxcfs", false, "Enable Lxcfs to make container to isolate /proc")
flagSet.StringVar(&cfg.LxcfsBinPath, "lxcfs", "/usr/local/bin/lxcfs", "Specify the path of lxcfs binary")
flagSet.StringVar(&cfg.LxcfsHome, "lxcfs-home", "/var/lib/lxc/lxcfs", "Specify the mount dir of lxcfs")
flagSet.StringVar(&cfg.DefaultRegistry, "default-registry", "registry.hub.docker.com/library/", "Default Image Registry")
flagSet.StringVar(&cfg.DefaultRegistry, "default-registry", "registry.hub.docker.com", "Default Image Registry")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not got your point yet, why we need split this params into two part? can you explain it, thanks a lot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for fix #892, I will add this issue into my commit.

@HusterWan HusterWan merged commit 92579c3 into AliyunContainerService:master Mar 21, 2018
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.

4 participants