Skip to content

Remove DockerTrustSuite to docker/cli e2e tests#36515

Merged
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:migrate-trusted-suite
Mar 21, 2018
Merged

Remove DockerTrustSuite to docker/cli e2e tests#36515
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:migrate-trusted-suite

Conversation

@vdemeester
Copy link
Copy Markdown
Member

@vdemeester vdemeester commented Mar 7, 2018

Remove DockerTrustSuite as it's client-side only and migrate those tests to docker/cli e2e tests.

See docker/cli#924

cc @endophage @n4ss

Still wip as there is still 6 tests to migrate and removing DockerTrustSuite altogether

Signed-off-by: Vincent Demeester [email protected]

@vdemeester vdemeester requested review from dnephin and thaJeztah March 7, 2018 17:08
@vdemeester vdemeester mentioned this pull request Mar 7, 2018
Merged
dnephin
dnephin reviewed Mar 7, 2018
View reviewed changes
Comment thread integration-cli/docker_cli_build_test.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Mar 7, 2018

Choose a reason for hiding this comment

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

This looks like a CLI-only test. I'll port this to a cli unit test so you can remove it here.

@vdemeester vdemeester force-pushed the migrate-trusted-suite branch from 4532dc7 to 7ef9f46 Compare March 7, 2018 17:55
@dnephin dnephin mentioned this pull request Mar 7, 2018
Merged
dnephin
dnephin reviewed Mar 7, 2018
View reviewed changes
Comment thread integration-cli/docker_cli_build_test.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Mar 7, 2018

Choose a reason for hiding this comment

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

Added to docker/cli in docker/cli#925

@vdemeester vdemeester force-pushed the migrate-trusted-suite branch from 7ef9f46 to a7828a4 Compare March 7, 2018 18:28
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 7, 2018 •
edited
Loading

Codecov Report

Merging #36515 into master will increase coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36515      +/-   ##
==========================================
+ Coverage   34.85%   35.11%   +0.26%     
==========================================
  Files         612      612              
  Lines       45445    45435      -10     
==========================================
+ Hits        15838    15956     +118     
+ Misses      27537    27373     -164     
- Partials     2070     2106      +36

@vdemeester vdemeester force-pushed the migrate-trusted-suite branch from a7828a4 to 35a97fd Compare March 7, 2018 18:36
dnephin
dnephin reviewed Mar 7, 2018
View reviewed changes
Comment thread internal/test/registry/registry.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Mar 7, 2018

Choose a reason for hiding this comment

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

Can we leave these 3 files under integration-cli ?

I'm not sure we want this for integration/.

We should just be running a v2 registry in a docker image instead of building a binary, or using a mock for integration testing. A lot of this seems to be an implementation of a client library, that should already exist as a library somewhere.

Comment thread integration/image/remove_test.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Mar 7, 2018

Choose a reason for hiding this comment

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

where is this used?

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Mar 8, 2018

Choose a reason for hiding this comment

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

oh it was to port some registry tests to this one but shouldn't have commited that 😅

@vdemeester vdemeester force-pushed the migrate-trusted-suite branch 2 times, most recently from d2af0f5 to c9898e3 Compare March 8, 2018 10:48
n4ss
n4ss reviewed Mar 8, 2018
View reviewed changes
Copy link
Copy Markdown

@n4ss n4ss left a comment

Choose a reason for hiding this comment

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

LGTM besides the small nit of the unused bit. Review done IRL :D

Comment thread integration/image/main_test.go Outdated
Copy link
Copy Markdown

@n4ss n4ss Mar 8, 2018

Choose a reason for hiding this comment

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

What's this for?

Copy link
Copy Markdown
Member

@dnephin dnephin Mar 13, 2018

Choose a reason for hiding this comment

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

remove this? I think it's unused.

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Mar 13, 2018

Choose a reason for hiding this comment

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

Oups.. forgot to commit+push 😅

@vdemeester vdemeester changed the title [wip] Remove DockerTrustSuite to docker/cli e2e tests Remove DockerTrustSuite to docker/cli e2e tests Mar 13, 2018
@vdemeester vdemeester force-pushed the migrate-trusted-suite branch from 31ca3b6 to 5433ceb Compare March 19, 2018 08:26
yongtang
yongtang approved these changes Mar 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Mar 21, 2018

experimental flakyness unrelated, merging 👼

@vdemeester vdemeester merged commit 8aa694c into moby:master Mar 21, 2018
@vdemeester vdemeester deleted the migrate-trusted-suite branch March 21, 2018 07:51
@thaJeztah thaJeztah mentioned this pull request Apr 10, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Apr 9, 2020
Closed
@vdemeester vdemeester mentioned this pull request Apr 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+3 more reviewers

@n4ss n4ss n4ss left review comments

@dnephin dnephin dnephin approved these changes

@yongtang yongtang yongtang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/security/trust area/testing status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vdemeester @dnephin @thaJeztah @n4ss @yongtang @GordonTheTurtle