Skip to content

Refactor dockerResolver with resolveDockerBase helper#4302

Merged
estesp merged 1 commit intocontainerd:masterfrom
songjiayang:update-docker-resolver
Jan 12, 2021
Merged

Refactor dockerResolver with resolveDockerBase helper#4302
estesp merged 1 commit intocontainerd:masterfrom
songjiayang:update-docker-resolver

Conversation

@songjiayang
Copy link
Copy Markdown

@songjiayang songjiayang commented Jun 4, 2020 •
edited
Loading

Recently I started reading the containerd 's source code, when I read at remotes/docker/resolver.go, find there are some codes are too similar and repetitive.

So I just add a resolveDockerBase helper for it , the changed code looks more simple.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 4, 2020

Build succeeded.

Zyqsempai
Zyqsempai reviewed Jun 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

Please sign your commit git commit -s to pass through CI.

@songjiayang songjiayang force-pushed the update-docker-resolver branch from 6af4f00 to ba0857f Compare June 5, 2020 01:58
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 5, 2020

Build succeeded.

@songjiayang
Copy link
Copy Markdown
Author

songjiayang commented Jun 5, 2020

@Zyqsempai thanks for your review, but there are some else failed cases, can you help me to figure out it.

@songjiayang songjiayang force-pushed the update-docker-resolver branch from ba0857f to 3c2536e Compare June 5, 2020 07:53
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 5, 2020

Build succeeded.

@songjiayang
Copy link
Copy Markdown
Author

songjiayang commented Jun 6, 2020

@Zyqsempai code updated, review again please.

@dmcgowan dmcgowan self-assigned this Jun 25, 2020
@dmcgowan dmcgowan added this to the 1.4 milestone Jun 25, 2020
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 6, 2020

needs rebase

@songjiayang
Copy link
Copy Markdown
Author

songjiayang commented Jul 9, 2020

sorry late reply, I will rebase now.

@songjiayang songjiayang force-pushed the update-docker-resolver branch 2 times, most recently from fa2ac41 to 3e042a2 Compare July 9, 2020 02:32
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 9, 2020

Build succeeded.

@songjiayang songjiayang force-pushed the update-docker-resolver branch from 3e042a2 to 679f35f Compare July 9, 2020 02:42
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 9, 2020

Build succeeded.

@songjiayang
Copy link
Copy Markdown
Author

songjiayang commented Jul 9, 2020

@AkihiroSuda code rebased, pr review again pls.

@AkihiroSuda AkihiroSuda requested review from dmcgowan and fuweid July 9, 2020 05:56
dmcgowan
dmcgowan requested changes Jul 9, 2020
View reviewed changes
Comment thread remotes/docker/resolver.go Outdated
@songjiayang songjiayang force-pushed the update-docker-resolver branch from 679f35f to 16ff398 Compare July 9, 2020 07:26
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 9, 2020

Build succeeded.

fuweid
fuweid previously requested changes Jul 10, 2020
View reviewed changes
Comment thread remotes/docker/resolver.go Outdated
Copy link
Copy Markdown
Member

@fuweid fuweid Jul 10, 2020

Choose a reason for hiding this comment

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

For remote.Fetcher/Pusher, it can be done with digest. But for Resolve API, it still needs to check Object.

@dmcgowan dmcgowan removed this from the 1.4 milestone Aug 3, 2020
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Sep 10, 2020

needs rebase

@songjiayang songjiayang force-pushed the update-docker-resolver branch from 16ff398 to 2475394 Compare September 14, 2020 03:23
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 14, 2020

Build succeeded.

Refactor dockerResolver to make code simple
5867c88 
1.add resolveDockerBase helper
2.dockerBase header copy with header.Clone()

Signed-off-by: songjiayang <[email protected]>
@songjiayang songjiayang force-pushed the update-docker-resolver branch from 2475394 to 5867c88 Compare September 14, 2020 03:38
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 14, 2020

Build succeeded.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Sep 14, 2020

@fuweid @dmcgowan PTAL

@dmcgowan dmcgowan dismissed fuweid’s stale review January 12, 2021 06:01

Resolved feedback

estesp
estesp approved these changes Jan 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 9db6aa6 into containerd:master Jan 12, 2021
@TBBle
Copy link
Copy Markdown
Contributor

TBBle commented Jan 12, 2021 •
edited
Loading

This seems to have broken all the unit tests, i.e. from https://github.com/containerd/containerd/runs/1689382546

--- FAIL: TestFetcherOpen (0.00s)
panic: assignment to entry in nil map [recovered]
	panic: assignment to entry in nil map

goroutine 6 [running]:
testing.tRunner.func1.1(0xba00a0, 0xcd9b10)
	/opt/hostedtoolcache/go/1.15.5/x64/src/testing/testing.go:1072 +0x46a
testing.tRunner.func1(0xc000001c80)
	/opt/hostedtoolcache/go/1.15.5/x64/src/testing/testing.go:1075 +0x636
panic(0xba00a0, 0xcd9b10)
	/opt/hostedtoolcache/go/1.15.5/x64/src/runtime/panic.go:975 +0x47a
net/textproto.MIMEHeader.Set(...)
	/opt/hostedtoolcache/go/1.15.5/x64/src/net/textproto/header.go:22
net/http.Header.Set(...)
	/opt/hostedtoolcache/go/1.15.5/x64/src/net/http/header.go:37
github.com/containerd/containerd/remotes/docker.dockerFetcher.open(0xc000089dc0, 0xcee2a0, 0xc00002c0d0, 0xc0001b0090, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/runner/work/containerd/containerd/src/github.com/containerd/containerd/remotes/docker/fetcher.go:152 +0x1e8
github.com/containerd/containerd/remotes/docker.TestFetcherOpen.func2(0x0)
	/home/runner/work/containerd/containerd/src/github.com/containerd/containerd/remotes/docker/fetcher_test.go:72 +0xe7
github.com/containerd/containerd/remotes/docker.TestFetcherOpen(0xc000001c80)
	/home/runner/work/containerd/containerd/src/github.com/containerd/containerd/remotes/docker/fetcher_test.go:94 +0x676
testing.tRunner(0xc000001c80, 0xc48b78)
	/opt/hostedtoolcache/go/1.15.5/x64/src/testing/testing.go:1123 +0x203
created by testing.(*T).Run
	/opt/hostedtoolcache/go/1.15.5/x64/src/testing/testing.go:1168 +0x5bc
FAIL	github.com/containerd/containerd/remotes/docker	0.029s

I suspect it's because #4855 has been reverted as part of this change.

Edit: Nope, it's because Header.Clone() on a nil Header returns nil, not http.Header{}.

#4933 appears to be the fix. At least it's passing the CI tests.

@TBBle TBBle mentioned this pull request Jan 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@estesp estesp estesp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@fuweid fuweid Awaiting requested review from fuweid

+1 more reviewer

@Zyqsempai Zyqsempai Zyqsempai left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dmcgowan dmcgowan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@songjiayang @AkihiroSuda @TBBle @dmcgowan @estesp @Zyqsempai @fuweid