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

add support for "registry-mirrors" and "insecure-registries" to buildkit #37852

Merged
merged 2 commits into from Sep 21, 2018

Conversation

Projects
None yet
9 participants
@AntaresS
Copy link
Contributor

AntaresS commented Sep 14, 2018

- What I did

  • update vendor with the recent patch from buildkit moby/buildkit#612
  • integrated with moby side where buildkit is being invoked.
    • pass registry-mirror and insecure-registries fields from daemon.json to buildkit in order to make use.
    • supports live reloading when daemon.json file changes.

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@AntaresS AntaresS requested a review from tonistiigi as a code owner Sep 14, 2018

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch from e6b79b6 to 9eb0664 Sep 14, 2018

@AntaresS

This comment has been minimized.

Copy link
Contributor Author

AntaresS commented Sep 14, 2018

@tonistiigi ptal when you have a chance

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch 5 times, most recently from c16325e to 9047d3b Sep 14, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 16, 2018

Codecov Report

Merging #37852 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37852      +/-   ##
==========================================
- Coverage   36.12%   36.12%   -0.01%     
==========================================
  Files         610      610              
  Lines       45083    45114      +31     
==========================================
+ Hits        16288    16296       +8     
- Misses      26555    26580      +25     
+ Partials     2240     2238       -2
update vendor
Signed-off-by: Anda Xu <anda.xu@docker.com>

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch from 9047d3b to 3c14b8b Sep 16, 2018

@yongtang yongtang requested a review from tiborvass Sep 17, 2018

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch 6 times, most recently from ac4e143 to 59482e0 Sep 17, 2018

@AntaresS

This comment has been minimized.

Copy link
Contributor Author

AntaresS commented Sep 18, 2018

@@ -8,6 +8,8 @@ import (
"sync"
"time"

"github.com/moby/buildkit/util/resolver"

This comment has been minimized.

@tiborvass

tiborvass Sep 19, 2018

Collaborator

nit: unneeded whitespace

This comment has been minimized.

@AntaresS

AntaresS Sep 20, 2018

Author Contributor

👍

)
// must trim "https://" prefix
for i, v := range daemon.configStore.Mirrors {
mirrors[i] = strings.Split(v, "//")[1]

This comment has been minimized.

@tiborvass

tiborvass Sep 19, 2018

Collaborator

this is going to panic if there is no //, which according to https://github.com/moby/moby/blob/master/registry/service_v2.go#L15 could happen.

This comment has been minimized.

@dmcgowan

dmcgowan Sep 19, 2018

Member

Cleaner to use strings.Index, maybe like

if idx := strings.Index(v, "://"); idx > 0 {
        v = v[idx+3:]
}
// set "insecure-registries"
for _, v := range daemon.configStore.InsecureRegistries {
m[v] = resolver.RegistryConf{
PlainHTTP: true,

This comment has been minimized.

@dmcgowan

dmcgowan Sep 19, 2018

Member

Does this need to get set if the mirror prefix is "http://"?

This comment has been minimized.

@AntaresS

AntaresS Sep 20, 2018

Author Contributor

@dmcgowan Ah I think I the "http://" prefix must be trimmed here as well. Otherwise, buildkit will add an extra prefix.

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch from 59482e0 to 0695415 Sep 20, 2018

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch from 0695415 to 0f24381 Sep 20, 2018

mirrors = make([]string, len(daemon.configStore.Mirrors))
m = map[string]resolver.RegistryConf{}
)
// must trim "https://" prefix

This comment has been minimized.

@dmcgowan

dmcgowan Sep 20, 2018

Member

Is http:// not valid in configuration?

This comment has been minimized.

@AntaresS

AntaresS Sep 20, 2018

Author Contributor

OK. Actually I just learned it is also valid 😅

This comment has been minimized.

@AntaresS

AntaresS Sep 20, 2018

Author Contributor

Added a trimming for that as well

add support of registry-mirrors and insecure-registries to buildkit
Signed-off-by: Anda Xu <anda.xu@docker.com>

@AntaresS AntaresS force-pushed the AntaresS:patch-buildkit branch from 0f24381 to 171d51c Sep 20, 2018

@dmcgowan

This comment has been minimized.

Copy link
Member

dmcgowan commented Sep 20, 2018

LGTM

@AntaresS

This comment has been minimized.

Copy link
Contributor Author

AntaresS commented Sep 20, 2018

@tiborvass @dmcgowan updated the PR based on the comments. Ptal when you have time.

@tiborvass
Copy link
Collaborator

tiborvass left a comment

LGTM

@tonistiigi tonistiigi merged commit b111647 into moby:master Sep 21, 2018

8 of 9 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project 36.12% (-0.01%) compared to 9ad4ef7
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 42252 has succeeded
Details
janky Jenkins build Docker-PRs 51028 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 11450 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4675 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 22304 has succeeded
Details
z Jenkins build Docker-PRs-s390x 11325 has succeeded
Details
@Jessie19877891

This comment has been minimized.

Copy link

Jessie19877891 commented on daemon/daemon.go in 171d51c Dec 9, 2018

Restore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.