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.

Sign up for GitHub

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

Jump to bottom

feature:add pouch rename #220

Merged
merged 1 commit into from Dec 5, 2017
Merged

feature:add pouch rename #220

merged 1 commit into from Dec 5, 2017

Conversation

zeppp
Copy link
Contributor

@zeppp zeppp commented Dec 4, 2017

Signed-off-by: zeppp zeppp1995@gmail.com
1.Describe what this PR did
pouch rename [container] [newName]rename a container with new name.
2.Does this pull request fix one issue?

3.Describe how you did it

4.Describe how to verify it

5.Special notes for reviews

@allencloud
Copy link
Collaborator

That is so great for us to have your contribution. @zeppp
The design is totally OK for me. While maybe we need do a little bit more.
I think we need to do :

1.define api in swagger.yml
2.generate API struct via in https://github.com/alibaba/pouch/blob/master/apis/README.md;
3.start to implement the API like you do.

I admit that will bother a lot, but I think it is also important to maintain a pouch in high quality. Would you like you to update this? And please feel free to tell us if you need any help. Thanks again.

@zeppp
Copy link
Contributor Author

zeppp commented Dec 5, 2017

I will define this in swagger soon.

@zeppp
Copy link
Contributor Author

zeppp commented Dec 5, 2017

I have defined the api in swagger.There is no changes in types because I didn't add new model in swagger.
Can you review it? @allencloud

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
apis/server/container_bridge.go Outdated
return err
}

resp.WriteHeader(http.StatusOK)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You defined 204, but here is 200.
Please make this consistent.

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
cli/rename.go Outdated
newName := args[1]

if err := apiClient.ContainerRename(container, newName); err != nil {
fmt.Fprintf(os.Stderr, "failed to rename container %s to %s %v \n", container, newName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not output error here, you can just return err, since https://github.com/alibaba/pouch/blob/master/cli/main.go#L31 will output error message.

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
cli/rename.go Outdated
rc.cmd = &cobra.Command{
Use: "rename [container] [newName]",
Short: "Rename a exist container with newName",
Args: cobra.MinimumNArgs(2),
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/MinimumNArgs/ExactArgs

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
cli/rename.go Outdated

rc.cmd = &cobra.Command{
Use: "rename [container] [newName]",
Short: "Rename a exist container with newName",
Copy link
Collaborator

@allencloud allencloud Dec 5, 2017 •
edited

Choose a reason for hiding this comment

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

Please add Long description and examples as well, like the create command does: https://github.com/alibaba/pouch/blob/master/cli/create.go#L28 and https://github.com/alibaba/pouch/blob/master/cli/create.go#L33 .

It helps a lot to generate cli docs. It is a feature merged just this morning. So maybe you have no idea about this. 😄

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
client/container.go Outdated
@@ -106,3 +106,14 @@ func (client *APIClient) ContainerStartExec(execid string, config *types.ExecSta

return client.hijack("/exec/"+execid+"/start", url.Values{}, config, header)
}

//ContainerRename rename a container.
Copy link
Collaborator

Choose a reason for hiding this comment

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

// ContainerRename renames a container.

add a whitespace after // and use renames please.

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
client/container.go Outdated
//ContainerRename rename a container.
func (client *APIClient) ContainerRename(name string, newName string) error {
q := url.Values{}
q.Add("newName", newName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In swagger.yml, you add a query which is named name, while here the query name is newName. Please make sure the name is consistent.

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
daemon/mgr/container.go Outdated
@@ -48,6 +48,9 @@ type ContainerMgr interface {

// Remove removes a container, it may be running or stopped and so on.
Remove(ctx context.Context, name string, option *ContainerRemoveOption) error

// Rename rename a container
Copy link
Collaborator

Choose a reason for hiding this comment

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

renames

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
daemon/mgr/container.go Outdated
@@ -364,6 +367,32 @@ func (cm *ContainerManager) List(ctx context.Context) ([]*types.ContainerInfo, e
return cis, nil
}

// Rename rename a exist container
Copy link
Collaborator

Choose a reason for hiding this comment

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

renames an existent

allencloud
allencloud reviewed Dec 5, 2017
View reviewed changes
daemon/mgr/container.go Outdated
)

if cm.NameToID.Get(newName).Exist() {
return errors.New("The newName already exists")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to use pkg https://github.com/alibaba/pouch/blob/master/pkg/httputils/http_error.go to wrap the error with status code 409 which is defined in your swagger.yml.

@zeppp
Copy link
Contributor Author

zeppp commented Dec 5, 2017

Thanks a lot! I will modify these soon.

@zeppp zeppp force-pushed the feature branch 2 times, most recently from 865b80a to b31c07b Compare December 5, 2017 10:48
@pouchrobot pouchrobot added size/XL and removed size/L labels Dec 5, 2017
@zeppp zeppp force-pushed the feature branch 2 times, most recently from cbe12cf to 5b584f7 Compare December 5, 2017 11:42
@pouchrobot
Copy link
Collaborator

@zeppp Thanks for your contribution. 🍻
Please sign off in each of your commits.

@zeppp
Copy link
Contributor Author

zeppp commented Dec 5, 2017

Work is done.
PTAL @allencloud

@allencloud
Copy link
Collaborator

LGTM, thanks a lot for your contribution and lots of update you made.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 5, 2017
@allencloud allencloud merged commit 1e6f0eb into AliyunContainerService:master Dec 5, 2017
@zeppp zeppp deleted the feature branch December 5, 2017 12:22
@allencloud allencloud mentioned this pull request Dec 8, 2017
Closed
55 tasks
@pouchrobot pouchrobot mentioned this pull request Dec 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allencloud allencloud allencloud left review comments

Assignees
No one assigned
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zeppp @allencloud @pouchrobot