Skip to content

Cleaned up "docker insert".#1942

Closed
ghost wants to merge 3 commits intomasterfrom
unknown repository
Closed

Cleaned up "docker insert".#1942
ghost wants to merge 3 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 20, 2013

Before, with Docker 0.6.2:

$ # Invalid Image ID.
$ sudo docker insert foo https://www.docker.io/static/img/docker-top-logo.png /foo

$ # Invalid URL.
$ docker insert 8dbd9 file:///foo /foo

$ # Success.
$ docker insert 8dbd9 https://www.docker.io/static/img/docker-top-logo.png /foo

5765ac1d7336c1d805f04e745b7fdc9583bb23202a825c6e26c933b636a357cd
5765ac1d7336: 

$

After:

$ # Invalid Image ID.
$ docker insert foo https://www.docker.io/static/img/docker-top-logo.png /foo
2013/09/20 05:01:46 Image does not exist: foo
$ # Invalid URL.
$ docker insert 8dbd9 file:///foo /foo
2013/09/20 05:02:39 Get file:///foo: unsupported protocol scheme "file"
$ # Success.
$ docker insert 8dbd9 https://www.docker.io/static/img/docker-top-logo.png /foo
c51a1d935c17: Image created
$

Fixes #1130.

@ghost ghost assigned shykes Sep 20, 2013
@keeb-zz
Copy link
Copy Markdown
Contributor

keeb-zz commented Sep 20, 2013

cc @shykes, this issue was automatically assigned to you by Gordon

vieux
vieux reviewed Sep 20, 2013
View reviewed changes
Comment thread server.go Outdated
Copy link
Copy Markdown
Contributor

@vieux vieux Sep 20, 2013

Choose a reason for hiding this comment

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

Do we really want the : Image created ? not sure if we want to be consistent with other commands.
In any case, could you keep using FormatStatus ?

out.Write(sf.FormatStatus(utils.TruncateID(img.ID), "Image created"))

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 23, 2013

Is this good to go?

@ghost ghost closed this Sep 27, 2013
@gurjeet
Copy link
Copy Markdown
Contributor

gurjeet commented Oct 29, 2013

@dsissitka Has this been merged?!!

Also, what is the averseness to allowing 'insert' of local files. It seems awkward that Docker would allow downloading files from internet and inserting them into images, but the local files are not allowed to be added to the images!

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 29, 2013

Afraid not. I couldn't get the Docker folks to review it so I moved on.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Oct 30, 2013

@dsissitka I apologize, we have been overwhelmed but are slowly but surely catching up on the backlog.

Let's get this merged this week. /cc @vieux @crosbymichael

@gurjeet
Copy link
Copy Markdown
Contributor

gurjeet commented Oct 30, 2013

@shykes Would it be a welcome feature to allow adding files from local machine, too. For now I am using the workaround of running a local static web server and then asking Docker to use a URL from that server to insert the local file. (and then of course kill the static web server).

/cc @vieux @crosbymichael

@creack
Copy link
Copy Markdown
Contributor

creack commented Nov 4, 2013

@dsissitka This looks good, but would you mind adding a couple of tests to enforce the behavior?

@creack creack reopened this Nov 4, 2013
@ghost ghost assigned creack Nov 4, 2013
@creack
Copy link
Copy Markdown
Contributor

creack commented Nov 4, 2013

assigning to me

@ghost
Copy link
Copy Markdown
Author

ghost commented Nov 5, 2013

Afraid not. I no longer have a Docker development environment setup.

@jadeallenx
Copy link
Copy Markdown

jadeallenx commented Nov 6, 2013

@creack I could add the tests if you want.

@creack
Copy link
Copy Markdown
Contributor

creack commented Nov 7, 2013

@mrallen1 This would be great indeed :) Thank you very much!

@jadeallenx
Copy link
Copy Markdown

jadeallenx commented Nov 7, 2013

Cool I will start working on it tonight.

jadeallenx pushed a commit to jadeallenx/docker that referenced this pull request Nov 8, 2013
@dsissitka
Make "docker insert" errors obvious
5957dd9 
Closes moby#1130
See also moby#1942
@jadeallenx jadeallenx mentioned this pull request Nov 8, 2013
Merged
@creack
Copy link
Copy Markdown
Contributor

creack commented Nov 8, 2013

Closing in favor of #2603

@creack creack closed this Nov 8, 2013
@jadeallenx jadeallenx mentioned this pull request Nov 13, 2013
Closed
@ghost ghost unassigned creack Jul 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

docker insert fails quietly, should report failure

7 participants

@keeb-zz @gurjeet @shykes @creack @jadeallenx @vieux @dsissitka