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

enhance: add more field in 'pouch info' command #1238

Merged
merged 1 commit into from
May 9, 2018

Conversation

ZouRui89
Copy link
Contributor

@ZouRui89 ZouRui89 commented Apr 27, 2018

Signed-off-by: Zou Rui [email protected]

Ⅰ. Describe what this PR did

This pr adds the value assertion of 'Architecture/Images/LoggingDriver/RegistryConfig' on daemon side, in which case 'pouch info' command wil output more field value.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

[root@instance-1 zouruicloud]# pouch info
Containers: 0
Running: 0
Paused: 0
Stopped: 0
Images: 1
ID:
Name: instance-1
Server Version: 0.4.0
Storage Driver: overlayfs
Driver Status: []
Logging Driver:
Cgroup Driver:
runc:
containerd:
Security Options: []
Kernel Version: 3.10.0-693.11.1.el7.x86_64
Operating System: "CentOS Linux 7 (Core)"
OSType: linux
Architecture: amd64
HTTP Proxy:
HTTPS Proxy:
Registry: https://index.docker.io/v1/
Experimental: false
Debug: false
Labels:
node_ip=10.142.0.2
SN=GoogleCloud-0F7AA324BDD10D51358D5410E1D02814
CPUs: 1
Total Memory: 3.456 GiB
Pouch Root Dir: /var/lib/pouch
LiveRestoreEnabled: true
LxcfsEnabled: false
Daemon Listen Addresses: [unix:///var/run/pouchd.sock]

Ⅴ. Special notes for reviews

@ZouRui89 ZouRui89 requested review from HusterWan and removed request for HusterWan April 27, 2018 06:04
@ZouRui89 ZouRui89 removed the request for review from HusterWan April 27, 2018 06:05
@pouchrobot pouchrobot added size/M and removed size/S labels Apr 27, 2018
@ZouRui89 ZouRui89 force-pushed the info branch 2 times, most recently from 1ed8bab to 3373790 Compare April 27, 2018 08:47
@@ -96,8 +99,13 @@ func (mgr *SystemManager) Info() (types.SystemInfo, error) {
OSName = osName
}

images, err := mgr.imageMgr.ListImages(context.Background(), "")
if err != nil {
logrus.Warnf("failed to get image info")
Copy link
Collaborator

Choose a reason for hiding this comment

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

this error log seems improper. You did not describe the error correctly, and did not log the detailed error message, right?

How about logrus.Warnf("failed to list images: %v", err)? @ZouRui89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The filed 'image' corresponds to the number of images, in which case I think logrus.Warnf("failed to get image info: %v", err) is better. WDYT? @allencloud

@allencloud
Copy link
Collaborator

I like your work. LGTM if some update according to code comment. @ZouRui89

DefaultLogConfig types.HostConfigAO0LogConfig `json:"default-log-config, omitempty"`

// RegistryService
RegistryService types.RegistryServiceConfig `json:"registry-service, omitempty" `
Copy link
Collaborator

Choose a reason for hiding this comment

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

We add this field in the code, however I am afraid we have not add this field's assignment in manager's code, right? Do we have a plan to add this field assignment? @ZouRui89

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 9, 2018
@allencloud allencloud merged commit 819c4d3 into AliyunContainerService:master May 9, 2018
@ZouRui89 ZouRui89 deleted the info branch May 16, 2018 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/cli LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants