feature: add "pouch rm" #214

skoowoo · 2017-12-04T03:08:30Z

"pouch rm" is used to remove a container which is stopped or created,
if a container is running, you can use "pouch rm -f" to force to remove
the container.

Signed-off-by: skoo87 marckywu@gmail.com

allencloud · 2017-12-04T03:49:52Z

cli/rm.go

+	r.cli = c
+	r.cmd = &cobra.Command{
+		Use:   "rm container",
+		Short: "remove a container",


I think this command supports removing multiple containers. Could we change Short message to Remove one or more containers to be more precise?

allencloud · 2017-12-04T03:55:55Z

client/container.go

+	defer ensureCloseReader(resp)
+
+	if resp.StatusCode != http.StatusNoContent {
+		body, err := ioutil.ReadAll(resp.Body)


Is there any necessary for us to add a new struct in swagger.yml, for exmaple ContainerRmResponse or something like that? And maybe this will make API data transferring more structed.

if remove success, there is no content in body as same as docker. Only ocurring error, the body contain the error message.
like this:
{"message":"container: 26ecc8f964f2831a4106d79808af2995a3517f0277bcd398a92c590cba0aeee7 is not stopped, can't remove it"}

if string(body) is empty string, this function will return an error. Is it reasonable?
Actually if resp.StatusCode != http.StatusNoContent returns true, it means this is a non-2xx response. Then the error message will not be in resp, I think it is in err of resp, err := client.delete("/images/"+name, q). This is guaranteed by HTTP framework. So I am wondering if this block is reasonable. @HusterWan @skoo87

Not as you said, if resp.StatusCode != http.StatusNoContent returns true, it means pouchd returns non-204 response. pouchd server remove container success, will returns 204 status code. if not 204, that is remove container failed. you should look at the server api code. the action is same as docker, it's reasonable.

I am afraid I was wrong. I search moby's client code, in its cli, when requesting, he will do like this:

func (cli *Client) sendRequest(ctx context.Context, method, path string, query url.Values, body io.Reader, headers headers) (serverResponse, error) { req, err := cli.buildRequest(method, cli.getAPIPath(path, query), body, headers) if err != nil { return serverResponse{}, err } resp, err := cli.doRequest(ctx, req) if err != nil { return resp, err } // !!!!!!!! this part is important, transfer error in body to the second return parameter of func if err := cli.checkResponseErr(resp); err != nil { return resp, err } return resp, nil }

Yes, you are right. I realized it's a problem after I answer you yesterday.

allencloud · 2017-12-05T04:54:40Z

apis/server/container_bridge.go

+
+	option := &mgr.ContainerRemoveOption{
+		Force:  httputils.BoolValue(req, "force"),
+		Volume: httputils.BoolValue(req, "v"),


I do not think we have implemented remove flags v and l.
Can we remove both two, or mark this and implement them in the future?

If the answer is latter one, I think we had better track this in an issue or mark that in code comments.

allencloud · 2017-12-05T04:57:04Z

Shall we add this API in swagger.yml ?

skoowoo · 2017-12-06T02:41:21Z

there is not structure what be needed to define in swagger.

allencloud · 2017-12-06T03:49:01Z

I agree with you that no struct needs added. While I am afraid we need to the api things like:

paths:
  /_ping:
    get:
      summary: ""
      description: ""
      responses:
        200:
          description: "no error"
          schema:
            type: "string"
            example: "OK"
        500:
          description: "server error"
          schema:
            $ref: '#/definitions/Error'

After this, we can generate API doc with tools. If not, this API will not be generated in API docs.

Auto-generation of API documents I have discussed with @CodeJuan, and will work it out soon.

skoowoo · 2017-12-06T05:47:51Z

I agree with you, I updated. @allencloud

skoowoo · 2017-12-07T08:38:40Z

please review it, thanks @allencloud

allencloud · 2017-12-07T08:46:31Z

apis/swagger.yml

-
+  /containers/{name}:
+    post:
+      summary: "Remove one or more containers"


This api only removes one container. So if Remove one container is better?

allencloud · 2017-12-07T08:48:02Z

cli/rm.go

+	r.cmd = &cobra.Command{
+		Use:   "rm container",
+		Short: "Remove one or more containers",
+		Args:  cobra.MinimumNArgs(1),


Add Long and Example in command definition like https://github.com/alibaba/pouch/blob/master/cli/create.go#L28-L33 ?
Then we can automatically generate docs? How about that?

allencloud · 2017-12-07T08:53:50Z

daemon/mgr/container.go

 func (mgr *ContainerManager) Remove(ctx context.Context, name string, option *ContainerRemoveOption) error {
+	c, err := mgr.container(name)


In the future, I think we need to classify a StatusNotFound code here, since we may not find container by input name.
We need to do this, in follow-up pull requests.

allencloud · 2017-12-07T08:58:22Z

daemon/mgr/container.go

+	defer c.Unlock()
+
+	if !c.IsStopped() && !c.IsCreated() && !option.Force {
+		return fmt.Errorf("container: %s is not stopped, can't remove it", c.ID())


When removing a running container without flag force, I think pouchd would return an error container: %s is not stopped, can't remove it.
I suggest using fmt.Errorf("container %s is not stopped, cannot remove it without flag force", c.ID()) to tell others to use force flag.

allencloud · 2017-12-07T15:47:48Z

apis/swagger.yml

@@ -298,7 +298,24 @@ paths:
          schema:
           $ref: "#/definitions/Error"
      tags: ["Container"]
-
+  /containers/{name}:
+    post:


this part should be delete, not post.

allencloud · 2017-12-07T15:48:19Z

cli/rm.go

+Remove a container object in Pouchd.
+If a container is stopped or created, you can remove it. 
+If the container is running, you can also remove it with flag force.
+When the container be removed, the all resource of the container will


"pouch rm" is used to remove a container which is stopped or created, if a container is running, you can use "pouch rm -f" to force to remove the container. Signed-off-by: skoo87 <marckywu@gmail.com>

skoowoo · 2017-12-08T02:26:45Z

updated @allencloud

allencloud · 2017-12-08T02:33:18Z

LGTM

pouchrobot added kind/feature size/L labels Dec 4, 2017

allencloud reviewed Dec 4, 2017

View reviewed changes

allencloud reviewed Dec 5, 2017

View reviewed changes

pouchrobot added conflict/needs-rebase size/XL and removed conflict/needs-rebase size/L labels Dec 6, 2017

allencloud reviewed Dec 7, 2017

View reviewed changes

feature: add "pouch rm"

8b8f29d

"pouch rm" is used to remove a container which is stopped or created, if a container is running, you can use "pouch rm -f" to force to remove the container. Signed-off-by: skoo87 <marckywu@gmail.com>

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 8, 2017

allencloud merged commit 2d31984 into AliyunContainerService:master Dec 8, 2017

allencloud mentioned this pull request Dec 8, 2017

[enhancement request] container management needs to polishment #137

Closed

55 tasks

allencloud mentioned this pull request Dec 9, 2017

[feature request] need CMD "pouch rm [-f]" #88

Closed

pouchrobot mentioned this pull request Dec 11, 2017

Weekly Report of Pouch from 2017-12-04 to 2017-12-11 #279

Closed

HusterWan mentioned this pull request Dec 12, 2017

[feature request] Need pouch rm ContainerID like docker rm #193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add "pouch rm" #214

feature: add "pouch rm" #214

skoowoo commented Dec 4, 2017

allencloud Dec 4, 2017

allencloud Dec 4, 2017

skoowoo Dec 4, 2017 •

edited

allencloud Dec 7, 2017

skoowoo Dec 7, 2017

allencloud Dec 7, 2017

skoowoo Dec 8, 2017

allencloud Dec 5, 2017

allencloud commented Dec 5, 2017

skoowoo commented Dec 6, 2017

allencloud commented Dec 6, 2017

skoowoo commented Dec 6, 2017

skoowoo commented Dec 7, 2017

allencloud Dec 7, 2017

allencloud Dec 7, 2017

allencloud Dec 7, 2017 •

edited

allencloud Dec 7, 2017

allencloud Dec 7, 2017

allencloud Dec 7, 2017

skoowoo commented Dec 8, 2017

allencloud commented Dec 8, 2017

		func (mgr ContainerManager) Remove(ctx context.Context, name string, option ContainerRemoveOption) error {
		c, err := mgr.container(name)

feature: add "pouch rm" #214

feature: add "pouch rm" #214

Conversation

skoowoo commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoowoo Dec 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Dec 5, 2017

skoowoo commented Dec 6, 2017

allencloud commented Dec 6, 2017

skoowoo commented Dec 6, 2017

skoowoo commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Dec 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoowoo commented Dec 8, 2017

allencloud commented Dec 8, 2017

skoowoo Dec 4, 2017 •

edited

allencloud Dec 7, 2017 •

edited