Rename a existing container #8570

brahmaroutu · 2014-10-14T19:01:30Z

Addresses #3036

Signed-off-by: Srini Brahmaroutu srbrahma@us.ibm.com

SvenDowideit · 2014-10-29T02:15:56Z

docs/man/docker-rename.1.md

+There are no available options.
+
+# HISTORY
+October 2014, updated by Sven Dowideit <SvenDowideit@home.org.au>


lies! i never wrote this ;)

Oops let me see, It may be just a cut and paste issue.

Sorry, It is a cut and paste errors(now removed).

SvenDowideit · 2014-10-29T02:17:15Z

oh, yes, i would like this. Docs LGTM -

now for an implementation question.

what happens when you rename a container that has been linked to? does everything still hand together? (I presume so, but :))

brahmaroutu · 2014-10-29T04:46:37Z

About the links, they work after rename, I tested them manually and also have integration tests that I can add, please let me know. I see the link is actually added to hosts file using IP address and container name is not used.

SvenDowideit · 2014-10-30T01:05:24Z

nice - @jamtur01 @fredlf

plus you need core review, @crosbymichael @tiborvass ?

fredlf · 2014-11-03T17:26:48Z

docker/flags.go

@@ -79,6 +79,7 @@ func init() {
 			{"ps", "List containers"},
 			{"pull", "Pull an image or a repository from a Docker registry server"},
 			{"push", "Push an image or a repository to a Docker registry server"},
+			{"rename", "Rename a existing container"},


Should be "an existing", here and below.

fredlf · 2014-11-03T17:29:14Z

Docs LGTM from me once the grammar mistake is fixed. Ping @jamtur01 @ostezer.

Still needs core review. Ping @crosbymichael @tiborvass

Many thanks for the contribution.

tiborvass · 2014-11-03T18:01:00Z

@brahmaroutu Thanks for your contribution.

I'd like to note though that this is not a proposal (hence my editing of the title). A proposal is either in a form of an issue explaining what are the changes needed and why, OR a PR to the documentation explaining what would change if it were implemented (see #8859 as an example).

Design needs to be reviewed with @shykes. Sorry if it's taking longer than expected! I'll make sure we review this in the next design review session.

brahmaroutu · 2014-11-03T18:22:22Z

@fredlf Sorry, I made that minor change to docs.
@tiborvass Thanks for reviewing, I understand such changes require more diligent review considerations.

fredlf · 2014-11-03T19:11:26Z

No problem, and thanks!

shykes · 2014-12-23T18:40:12Z

Design review with @icecrime

I approve docker rename OLD NEW. I haven't looked at the implementation. In particular please make sure edge cases are properly handled in the underlying storage (graphdb etc).

shykes · 2014-12-23T18:41:19Z

#uxapproved

jessfraz · 2014-12-23T18:47:05Z

integration-cli/docker_cli_links_test.go

+		t.Fatal(out, err)
+	}
+
+	dockerCmd(t, "kill", idA)


I don't think these are needed if you deleteAllContainers

LK4D4 · 2015-01-09T16:49:22Z

ping @jfrazelle @icecrime @tiborvass

icecrime · 2015-01-09T18:53:00Z

daemon/rename.go

+		return job.Errorf("Failed to delete container %q: %v", old_name, err)
+	}
+	if _, err := daemon.reserveName(container.ID, new_name); err != nil {
+		return job.Errorf("Error when allocating new name: %s", err)


There is a major issue here, as error is returned without releasing the lock (same for the code path right above):

$ docker run --name test busybox true $ docker rename test " " Error response from daemon: Error when allocating new name: Invalid container name ( ), only [a-zA-Z0-9][a-zA-Z0-9_.-] are allowed FATA[0000] Error: failed to rename container named test $ docker ps -a # Hangs (probably daemon in deadlock)

I think we should use defer container.Unlock() to play safe.

added defer on Unlock. thanks for pointing it out.

icecrime · 2015-01-09T18:53:19Z

Not LGTM: there is a blocking issue (see my comment).

brahmaroutu · 2015-01-09T19:16:53Z

Please let me know if I change it to the first ever PUT on Docker server, makes sense, instead of POST?

icecrime · 2015-01-13T00:45:26Z

Can we get an API maintainer input on the method to use so we can move on with this cool PR? Ping @vieux @jfrazelle!

vieux · 2015-01-13T01:03:53Z

I would use GET parameter:

"/containers/{name:.*}/rename?name={newname}"

brahmaroutu · 2015-01-13T01:55:34Z

@vieux Can you also let me know if I should change it to PUT call?

vieux · 2015-01-13T02:00:56Z

Sorry I meant a POST /containers/{name:.*}/rename?name={newname}

We don't have any PUT on the API, we use only POST, GET or DELETE to I would stay on the POST

Closes moby#3036 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

brahmaroutu · 2015-01-13T04:09:50Z

@vieux @cpuguy83 @LK4D4 Please review, code is updated as per your comments

LK4D4 · 2015-01-13T04:29:33Z

LGTM

thaJeztah · 2015-01-13T07:38:18Z

fwiw; since I'm the one coming up with PUT originally; I'm fine with POST for consistency with the rest of the API.

(If the need ever rises to be more "strict", I think that would require a complete review of the whole API)

jessfraz · 2015-01-13T18:36:10Z

LGTM

Rename a existing container

vieux · 2015-01-13T19:08:15Z

No API bump ?
No New section in docs/sources/reference/api/docker_remote_api.md ?

jessfraz · 2015-01-13T19:10:12Z

@vieux making patch now

vieux · 2015-01-13T19:11:53Z

@jfrazelle thank you.

vieux · 2015-01-13T19:12:48Z

It's easier for us when there is doc, every week we can look and see if we need to add the new endpoints in swarm.

brahmaroutu · 2015-01-13T19:20:30Z

@jfrazelle thanks, let me know if I can help.

The "or rename" part was removed from the error-message, because renaming wasn't possible at the time. Now that moby#8570 is merged, renaming existing containers is possible. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

brahmaroutu · 2015-01-14T00:38:46Z

@borromeotlhs there are not many tests testing 409 and I did not add one. I can quickly add a test if you want but I need a issue to do so.

brahmaroutu force-pushed the rename_container_3036 branch 4 times, most recently from cb27f73 to 73a69ac Compare October 16, 2014 18:24

jamtur01 added the /project/doc label Oct 16, 2014

SvenDowideit reviewed Oct 29, 2014
View reviewed changes

brahmaroutu force-pushed the rename_container_3036 branch 2 times, most recently from 1f39e4c to 62892b9 Compare October 29, 2014 03:09

SvenDowideit changed the title ~~rename a existing container~~ Proposal: Rename a existing container Oct 30, 2014

fredlf reviewed Nov 3, 2014
View reviewed changes

tiborvass changed the title ~~Proposal: Rename a existing container~~ Rename a existing container Nov 3, 2014

brahmaroutu force-pushed the rename_container_3036 branch from 62892b9 to e0e9507 Compare November 3, 2014 18:20

crosbymichael added the UX label Nov 13, 2014

brahmaroutu force-pushed the rename_container_3036 branch 3 times, most recently from 50c38fb to 7712f98 Compare December 2, 2014 20:31

brahmaroutu force-pushed the rename_container_3036 branch from 7712f98 to d06850a Compare December 19, 2014 19:02

shykes removed UX labels Dec 23, 2014

jessfraz reviewed Dec 23, 2014
View reviewed changes

icecrime reviewed Jan 9, 2015
View reviewed changes

brahmaroutu force-pushed the rename_container_3036 branch from 62ecf5c to 3820e9d Compare January 9, 2015 19:12

rename a existing container

21a809d

Closes moby#3036 Signed-off-by: Srini Brahmaroutu <srbrahma@us.ibm.com>

brahmaroutu force-pushed the rename_container_3036 branch from 3820e9d to 21a809d Compare January 13, 2015 03:27

jessfraz pushed a commit that referenced this pull request Jan 13, 2015

Merge pull request #8570 from brahmaroutu/rename_container_3036

b9e42d6

Rename a existing container

jessfraz merged commit b9e42d6 into moby:master Jan 13, 2015

cpuguy83 mentioned this pull request Jan 13, 2015

Option to override container name #4212

Closed

thaJeztah mentioned this pull request Jan 13, 2015

Mention "or rename" again in error-message. #10070

Merged

brahmaroutu deleted the rename_container_3036 branch January 15, 2015 18:03

aanand mentioned this pull request Jan 23, 2015

Drop intermediate container and use renaming instead docker/compose#874

Closed

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

VynceMontgomery mentioned this pull request Jun 3, 2015

command line completion should be aware of 'rename' #13687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename a existing container #8570

Rename a existing container #8570

brahmaroutu commented Oct 14, 2014

SvenDowideit Oct 29, 2014

brahmaroutu Oct 29, 2014

brahmaroutu Oct 29, 2014

SvenDowideit commented Oct 29, 2014

brahmaroutu commented Oct 29, 2014

SvenDowideit commented Oct 30, 2014

fredlf Nov 3, 2014

fredlf commented Nov 3, 2014

tiborvass commented Nov 3, 2014

brahmaroutu commented Nov 3, 2014

fredlf commented Nov 3, 2014

shykes commented Dec 23, 2014

shykes commented Dec 23, 2014

jessfraz Dec 23, 2014

LK4D4 commented Jan 9, 2015

icecrime Jan 9, 2015

brahmaroutu Jan 9, 2015

icecrime commented Jan 9, 2015

brahmaroutu commented Jan 9, 2015

icecrime commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

LK4D4 commented Jan 13, 2015

thaJeztah commented Jan 13, 2015

jessfraz commented Jan 13, 2015

vieux commented Jan 13, 2015

jessfraz commented Jan 13, 2015

vieux commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

brahmaroutu commented Jan 14, 2015

Rename a existing container #8570

Rename a existing container #8570

Conversation

brahmaroutu commented Oct 14, 2014

SvenDowideit Oct 29, 2014

Choose a reason for hiding this comment

brahmaroutu Oct 29, 2014

Choose a reason for hiding this comment

brahmaroutu Oct 29, 2014

Choose a reason for hiding this comment

SvenDowideit commented Oct 29, 2014

brahmaroutu commented Oct 29, 2014

SvenDowideit commented Oct 30, 2014

fredlf Nov 3, 2014

Choose a reason for hiding this comment

fredlf commented Nov 3, 2014

tiborvass commented Nov 3, 2014

brahmaroutu commented Nov 3, 2014

fredlf commented Nov 3, 2014

shykes commented Dec 23, 2014

shykes commented Dec 23, 2014

jessfraz Dec 23, 2014

Choose a reason for hiding this comment

LK4D4 commented Jan 9, 2015

icecrime Jan 9, 2015

Choose a reason for hiding this comment

brahmaroutu Jan 9, 2015

Choose a reason for hiding this comment

icecrime commented Jan 9, 2015

brahmaroutu commented Jan 9, 2015

icecrime commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

LK4D4 commented Jan 13, 2015

thaJeztah commented Jan 13, 2015

jessfraz commented Jan 13, 2015

vieux commented Jan 13, 2015

jessfraz commented Jan 13, 2015

vieux commented Jan 13, 2015

vieux commented Jan 13, 2015

brahmaroutu commented Jan 13, 2015

brahmaroutu commented Jan 14, 2015