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 CriEnabled field in api and show this in cli #1918

Merged
merged 1 commit into from
Jul 30, 2018

Conversation

allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

In daemon's API side, we need to provide a user-friendly way for user to check if the pouchd is opening for CRI part.

Otherwise, user can only use ps aux | grep pouchd | grep cri-enable to check, which is quite inconvenient. In addition, if a pouch client can only access pouchd remotely, no info can be gotten with this way. So, I think we need to add CriEnabled field in api and show this in cli.

Ⅱ. Does this pull request fix one issue?

fixes #1908

Ⅲ. Describe how you did it

  1. Add in swagger.yml
  2. return in daemon side;
  3. show in cli side

Ⅳ. Describe how to verify it

execute pouch info, you will see CriEnabled: false if no cri-enabled, otherwise, true.

Ⅴ. Special notes for reviews

none

@allencloud allencloud force-pushed the add-crienabled branch 2 times, most recently from a6fcec4 to ebbd0d1 Compare July 26, 2018 17:35
@pouchrobot pouchrobot added size/L and removed size/S labels Jul 26, 2018
@codecov-io
Copy link

codecov-io commented Jul 26, 2018

Codecov Report

Merging #1918 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1918      +/-   ##
==========================================
+ Coverage    62.8%   62.81%   +<.01%     
==========================================
  Files         200      200              
  Lines       15567    15568       +1     
==========================================
+ Hits         9777     9779       +2     
- Misses       4544     4545       +1     
+ Partials     1246     1244       -2
Flag Coverage Δ
#criv1alpha1test 33.28% <0%> (ø) ⬆️
#criv1alpha2test 33.75% <0%> (+0.02%) ⬆️
#integrationtest 37.96% <100%> (+0.03%) ⬆️
#unittest 20.7% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/system.go 75.21% <100%> (+0.21%) ⬆️
ctrd/image.go 79.2% <0%> (-1.99%) ⬇️
cri/v1alpha2/cri.go 66.13% <0%> (+0.17%) ⬆️
ctrd/watch.go 75.75% <0%> (+3.03%) ⬆️
cri/stream/httpstream/spdy/upgrade.go 60% <0%> (+5.71%) ⬆️

"--enable-cri")
c.Assert(err, check.IsNil)

// Check pull image with default registry using the registry specified in daemon.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not appropriate 😄

@YaoZengzeng
Copy link
Contributor

LGTM, except the minor issue mentioned above :)

@allencloud
Copy link
Collaborator Author

Thanks for your review. @YaoZengzeng
I have removed the comment. Until it passes the CI, I think we could merge this.

@starnop
Copy link
Contributor

starnop commented Jul 30, 2018

LGTM.

@allencloud allencloud merged commit 8335362 into AliyunContainerService:master Jul 30, 2018
@allencloud allencloud deleted the add-crienabled branch July 30, 2018 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] add IsCRIEnabled field in /info API and show it in pouch info cli
5 participants