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

refactor: modify the definition of ContainerPlugin interface #2316

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Oct 16, 2018

Signed-off-by: zhuangqh zhuangqhc@gmail.com
Signed-off-by: Wei Fu fhfuwei@163.com

Ⅰ. Describe what this PR did

modify the definition of ContainerPlugin interface: PreCreate

Type *types.ContainerCreateConfig would be more generic, while io.ReadCloser read only stream data.

The CRI need to support the rich-mode container so that the create_hook_plugin should be handled in container_mgr.

Ⅱ. Does this pull request fix one issue?

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

redesign the interface

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Oct 16, 2018

Codecov Report

Merging #2316 into master will increase coverage by 0.17%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2316      +/-   ##
==========================================
+ Coverage    67.2%   67.38%   +0.17%     
==========================================
  Files         213      213              
  Lines       17511    17510       -1     
==========================================
+ Hits        11769    11799      +30     
+ Misses       4341     4319      -22     
+ Partials     1401     1392       -9
Flag Coverage Δ
#criv1alpha1test 32.24% <0%> (-0.03%) ⬇️
#criv1alpha2test 36.2% <0%> (+0.03%) ⬆️
#integrationtest 40.15% <0%> (+0.09%) ⬆️
#nodee2etest 33.73% <0%> (+0.12%) ⬆️
#unittest 23.2% <0%> (ø) ⬆️
Impacted Files Coverage Δ
apis/server/container_bridge.go 85.49% <ø> (+0.99%) ⬆️
daemon/mgr/spec_hook.go 24.56% <ø> (ø) ⬆️
daemon/mgr/container.go 57.49% <0%> (-0.24%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
cri/v1alpha2/cri.go 69.34% <0%> (+0.24%) ⬆️
ctrd/container.go 59.28% <0%> (+0.95%) ⬆️
daemon/containerio/container_io.go 75.95% <0%> (+1.09%) ⬆️
ctrd/image.go 78.94% <0%> (+2.19%) ⬆️
pkg/meta/store.go 62.5% <0%> (+2.34%) ⬆️
cri/v1alpha2/cri_wrapper.go 63.6% <0%> (+2.39%) ⬆️
... and 4 more

@zhuangqh zhuangqh changed the title fix: modify the definition of ContainerPlugin interface refactor: modify the definition of ContainerPlugin interface Oct 16, 2018
@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Oct 16, 2018
@zhuangqh zhuangqh requested a review from fuweid October 16, 2018 10:10

// ContainerPlugin defines places where a plugin will be triggered in container lifecycle
type ContainerPlugin interface {
// PreCreate defines plugin point where receives a container create request, in this plugin point user
// could change the container create body passed-in by http request body
PreCreate(io.ReadCloser) (io.ReadCloser, error)
PreCreate(*types.ContainerCreateConfig) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Interface signature and return value are all different from origin, some files missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xiaoxubeii You are right...

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
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.

LGTM

@fuweid fuweid merged commit 9a6fae1 into AliyunContainerService:master Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project kind/refactor size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants