Skip to content

Conversation

@wangforthinker
Copy link
Contributor

Signed-off-by: allen.wang [email protected]

Ⅰ. Describe what this PR did

it can create container with a given id.

Ⅱ. Does this pull request fix one issue?

fixes #2180

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

add test TestBasicWithSpecificID and TestInvalidSpecificID in api_container_create_test.go.

Ⅳ. Describe how to verify it

verify it by test.

Ⅴ. Special notes for reviews

none.

@wangforthinker wangforthinker force-pushed the feature/create-container-with-given-ID branch from 6822c14 to 95612ed Compare September 11, 2018 11:58
@allencloud allencloud changed the title support create a container with a given id feature: support create a container with a given id Sep 11, 2018
@allencloud
Copy link
Collaborator

Could you please first to fix the conflict there in this pull request? @wangforthinker

@wangforthinker wangforthinker force-pushed the feature/create-container-with-given-ID branch from f8bdc3d to bdbf90b Compare September 12, 2018 06:55
@pouchrobot pouchrobot added size/XL and removed size/L labels Sep 12, 2018
@wangforthinker wangforthinker force-pushed the feature/create-container-with-given-ID branch from bdbf90b to 1244439 Compare September 12, 2018 07:25
@codecov-io
Copy link

codecov-io commented Sep 12, 2018

Codecov Report

Merging #2231 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2231      +/-   ##
==========================================
+ Coverage    68.4%   68.48%   +0.07%     
==========================================
  Files         271      271              
  Lines       18263    18281      +18     
==========================================
+ Hits        12493    12519      +26     
+ Misses       4347     4344       -3     
+ Partials     1423     1418       -5
Flag Coverage Δ
#criv1alpha1test 31.82% <26.31%> (+0.02%) ⬆️
#criv1alpha2test 35.72% <26.31%> (-0.1%) ⬇️
#integrationtest 39.76% <100%> (+0.04%) ⬆️
#nodee2etest 33.38% <26.31%> (+0.06%) ⬆️
#unittest 25.49% <0%> (-0.03%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container.go 60.44% <100%> (+0.42%) ⬆️
apis/server/container_bridge.go 87.76% <100%> (+0.11%) ⬆️
ctrd/watch.go 78.78% <0%> (-4.55%) ⬇️
cri/v1alpha2/cri.go 68.53% <0%> (+0.23%) ⬆️
cri/v1alpha1/cri.go 61.03% <0%> (+0.32%) ⬆️
cri/v1alpha2/cri_wrapper.go 62.4% <0%> (+1.19%) ⬆️
daemon/mgr/container_utils.go 83.43% <0%> (+1.22%) ⬆️
pkg/meta/store.go 64.06% <0%> (+1.56%) ⬆️
daemon/logger/jsonfile/utils.go 73.17% <0%> (+1.62%) ⬆️

@allencloud
Copy link
Collaborator

Could you please rebase to the latest master? @wangforthinker
Since I found that in you commit have lots of changes from previous commits in the master.

fuweid
fuweid previously requested changes Sep 14, 2018
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

need to rebase @wangforthinker thanks

@wangforthinker wangforthinker force-pushed the feature/create-container-with-given-ID branch 3 times, most recently from 4234411 to 0a3d65a Compare October 24, 2018 11:35
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 4, 2018
@allencloud allencloud merged commit 8142201 into AliyunContainerService:master Nov 4, 2018
@wangforthinker wangforthinker deleted the feature/create-container-with-given-ID branch December 19, 2018 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. priority/P2 size/XL status/4-ready-to-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[proposal] create a container with a given ID

7 participants