Rename a existing container by brahmaroutu · Pull Request #8570 · moby/moby

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

Addresses #3036

Signed-off-by: Srini Brahmaroutu [email protected]

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

docs/man/docker-rename.1.md

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

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

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

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 <[email protected]>

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 <[email protected]>

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

docker/flags.go Outdated

Copy link

Contributor

fredlf Nov 3, 2014

Choose a reason for hiding this comment

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

Should be "an existing", here and below.

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

integration-cli/docker_cli_links_test.go Outdated

Copy link

Contributor

jessfraz Dec 23, 2014

Choose a reason for hiding this comment

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

I don't think these are needed if you deleteAllContainers

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 <[email protected]>

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

Conversation

brahmaroutu commented Oct 14, 2014

Uh oh!

SvenDowideit Oct 29, 2014

Choose a reason for hiding this comment

Uh oh!

brahmaroutu Oct 29, 2014

Choose a reason for hiding this comment

Uh oh!

brahmaroutu Oct 29, 2014

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Oct 29, 2014

Uh oh!

brahmaroutu commented Oct 29, 2014

Uh oh!

SvenDowideit commented Oct 30, 2014

Uh oh!

fredlf Nov 3, 2014

Choose a reason for hiding this comment

Uh oh!

fredlf commented Nov 3, 2014

Uh oh!

tiborvass commented Nov 3, 2014

Uh oh!

brahmaroutu commented Nov 3, 2014

Uh oh!

fredlf commented Nov 3, 2014

Uh oh!

shykes commented Dec 23, 2014

Uh oh!

shykes commented Dec 23, 2014

Uh oh!

jessfraz Dec 23, 2014

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 9, 2015

Uh oh!

icecrime Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

brahmaroutu Jan 9, 2015

Choose a reason for hiding this comment

Uh oh!

icecrime commented Jan 9, 2015

Uh oh!

brahmaroutu commented Jan 9, 2015

Uh oh!

icecrime commented Jan 13, 2015

Uh oh!

vieux commented Jan 13, 2015

Uh oh!

brahmaroutu commented Jan 13, 2015

Uh oh!

vieux commented Jan 13, 2015

Uh oh!

brahmaroutu commented Jan 13, 2015

Uh oh!

LK4D4 commented Jan 13, 2015

Uh oh!

thaJeztah commented Jan 13, 2015

Uh oh!

jessfraz commented Jan 13, 2015

Uh oh!

vieux commented Jan 13, 2015

Uh oh!

jessfraz commented Jan 13, 2015

Uh oh!

vieux commented Jan 13, 2015

Uh oh!

vieux commented Jan 13, 2015

Uh oh!

brahmaroutu commented Jan 13, 2015

Uh oh!

brahmaroutu commented Jan 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects