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 volume inspect on cli and client side #585

Merged
merged 1 commit into from
Jan 18, 2018
Merged

feature: add volume inspect on cli and client side #585

merged 1 commit into from
Jan 18, 2018

Conversation

ZouRui89
Copy link
Contributor

@ZouRui89 ZouRui89 commented Jan 16, 2018

Signed-off-by: Zou Rui 21751189@zju.edu.cn

1.Describe what this PR did
add volume inspect on cli and client side
2.Does this pull request fix one issue?

fixes part of #139
3.Describe how you did it

4.Describe how to verify it

Firstly, you should create a volume.
Then you verify whether or not 'volume inspect' command works.
Finally, you remove that volume and try the 'volume inspect' command again to see if the error returned indicates that the volume does not exist.

$ pouch volume create -d local -n pouch-volume -o size=100g
Driver:       local
Mountpoint:   
Name:         pouch-volume
Scope:        
CreatedAt:    

$ pouch volume inspect pouch-volume
Mountpoint:   /mnt/local/pouch-volume
Name:         pouch-volume
Scope:        
CreatedAt:    2018-1-17 14:09:30
Driver:       local

$ pouch volume rm pouch-volume
Removed: pouch-volume

$ pouch volume inspect pouch-volume
Error: {"message":"volume not found"}

5.Special notes for reviews

@codecov-io
Copy link

codecov-io commented Jan 16, 2018

Codecov Report

Merging #585 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage   13.71%   13.56%   -0.15%     
==========================================
  Files          64       64              
  Lines        3412     3449      +37     
==========================================
  Hits          468      468              
- Misses       2896     2933      +37     
  Partials       48       48
Impacted Files Coverage Δ
client/volume.go 0% <0%> (ø) ⬆️
cli/volume.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41efd76...9a333c6. Read the comment docs.

@allencloud
Copy link
Collaborator

I think this is a duplicate of #577. But we will take a look of which one is more proporiate to move on.

Could you add more API integration test and CLI integration test? @ZouRui89

@ZouRui89
Copy link
Contributor Author

Oh, I see. I will avoid duplicated work in the future by paying more attention to open PRs.

client/volume.go Outdated
func (client *APIClient) VolumeRemove(name string) error {
resp, err := client.delete("/volumes/"+name, nil)
ensureCloseReader(resp)

return err
}

//VolumeInspect inspects a volume.
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 white space after //.

}
path := "/volumes/create"
body := request.WithJSONBody(obj)
request.Post(path, body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make request.Post return error and do assert here, since this may error here but we ignore this.

c.Assert(resp.StatusCode, check.Equals, 200)
}

//TestVolumeInspectNotFound tests inspecting a volume which cannot be found.
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 white space after //.

//TestVolumeInspectNotFound tests inspecting a volume which cannot be found.
func (suite *APIVolumeInspectSuite) TestVolumeInspectNotFound(c *check.C) {
//Delete the volume "TestVolume" which has been created.
vol := "TestVolume"
Copy link
Collaborator

@allencloud allencloud Jan 17, 2018

Choose a reason for hiding this comment

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

Just define vol := NonExistentVolume, so we can check if the status code is 404 directly.

@allencloud
Copy link
Collaborator

Please help review the test part. @Letty5411

@allencloud
Copy link
Collaborator

In the future, we will test removing a used volume.
What will happen if we are removing a used volume?

@allencloud
Copy link
Collaborator

allencloud commented Jan 17, 2018

CI fails:

----------------------------------------------------------------------
FAIL: /go/src/github.com/alibaba/pouch/test/api_volume_inspect_test.go:50: APIVolumeInspectSuite.TestVolumeInspectNotFound
/go/src/github.com/alibaba/pouch/test/api_volume_inspect_test.go:56:
    c.Assert(resp.StatusCode, check.Equals, 404)
... obtained int = 500
... expected int = 404

I think we need to update code in network manager. @rudyfly

@rudyfly
Copy link
Collaborator

rudyfly commented Jan 17, 2018

Same pr with #577, can you merge these, and I will close my pr, I think this pr the volume's information is less, should add more information in the pouchd. Do you think so.

@allencloud
Copy link
Collaborator

PR #596 has merged. Please rebase this PR to the latest master branch, and fix the status code. Thanks a lot. @ZouRui89

Signed-off-by: Zou Rui <21751189@zju.edu.cn>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 18, 2018
@allencloud allencloud merged commit ac4b96a into AliyunContainerService:master Jan 18, 2018
Copy link
Collaborator

@rudyfly rudyfly left a comment

Choose a reason for hiding this comment

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

@@ -33,6 +33,7 @@ func (v *VolumeCommand) Init(c *Cli) {

c.AddCommand(v, &VolumeCreateCommand{})
c.AddCommand(v, &VolumeRemoveCommand{})
c.AddCommand(v, &VolumeInspectCommand{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my pr, I use 'info' instead of 'inspect', but because of consistenting with moby's, inspect is better.


// VolumeRemoveCommand is used to implement 'volume rm' command.
type VolumeRemoveCommand struct {
baseCommand
name string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add this variable?

func volumeInspectExample() string {
return `$ pouch volume inspect a02217023fc477876a5483100b87d84d8845d5ac7c0c706717f2ff6edc0cf425
Scope:
CreatedAt:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should display all of information what is exist in pouchd. So it should be added in pouchd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/storage kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants