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

feature: add a flag of pouchd to specify whether enable CRI #1118

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng [email protected]

Ⅰ. Describe what this PR did

With this PR, the CRI service of pouch will be off by default.

If we want to enable it, we could start pouchd like:

pouchd --enable-cri

Ⅱ. Does this pull request fix one issue?

fixes #1114

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@@ -22,6 +22,9 @@ type Config struct {
// Network config
NetworkConfg network.Config

// Whether enable cri manager.
CriEnabled bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a json name for this field, since it will also be included in the config file which in json, like json:"home-dir,omitempty".

@codecov-io
Copy link

codecov-io commented Apr 12, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1118     +/-   ##
========================================
+ Coverage    15.8%   16.7%   +0.9%     
========================================
  Files         171     165      -6     
  Lines        9619    8931    -688     
========================================
- Hits         1520    1492     -28     
+ Misses       7989    7334    -655     
+ Partials      110     105      -5
Impacted Files Coverage Δ
cli/inspect/inspector.go 53.52% <0%> (-19.62%) ⬇️
daemon/mgr/system.go 0% <0%> (ø) ⬆️
cli/inspect.go 0% <0%> (ø) ⬆️
apis/server/container_bridge.go
apis/server/network_bridge.go
apis/server/system_bridge.go
apis/server/image_bridge.go
apis/server/router.go
apis/server/server.go
apis/server/volume_bridge.go
... and 3 more

@YaoZengzeng
Copy link
Contributor Author

@allencloud PTAL

@@ -22,6 +22,9 @@ type Config struct {
// Network config
NetworkConfg network.Config

// Whether enable cri manager.
CriEnabled bool `json:"cri-enabled,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am afraid this field addition introduces some compatibility.

I see two similar fields:

CriEnabled bool `json:"cri-enabled,omitempty"`
IsLxcfsEnabled bool `json:"enable-lxcfs,omitempty"`

They are quite similar and have different. Could we change into IsCriEnabled bool json:"enable-cri,omitempty"``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't agree more :) be in consistent !

@allencloud
Copy link
Collaborator

Thanks for your work. @YaoZengzeng
LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 12, 2018
@allencloud allencloud merged commit 1bcbb04 into AliyunContainerService:master Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] add a flag to enable all of the CRI service
4 participants