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.

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

Do not alter json in docker save #6461

Closed
wants to merge 3 commits into from
Closed

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Jun 17, 2014

Fix #6455
Fix #6340
Close #6385

Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)
@vieux
Copy link
Contributor Author

vieux commented Jun 17, 2014

ping @vbatts what about this ?

Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)
@crosbymichael
Copy link
Contributor

Do you know of a good way to test this? Maybe validate the json structure by parsing?

Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)
@vieux
Copy link
Contributor Author

vieux commented Jun 17, 2014

@crosbymichael test added, look at commit 614c57c it's much more readable.

@vbatts
Copy link
Contributor

vbatts commented Jun 17, 2014

As for a way to unit test it, that may be a little more tricky.

root@jellyroll:/var/lib/docker/graph# docker save a9eb172552348a9a49180694790b33a1097f546456d041b6e82e4d7716ddb721 | tar xO -- a9eb172552348a9a49180694790b33a1097f546456d041b6e82e4d7716ddb721/json | md5sum
6191105cf0ea6f9273aa2de7b7fc6465  -
root@jellyroll:/var/lib/docker/graph# cat a9eb172552348a9a49180694790b33a1097f546456d041b6e82e4d7716ddb721/json | md5sum
715465f178b591dbe580e3da421bd3b4  -

Close, but they aren't the same. I'm thinking that there be a be a interface in image.Image like https://github.com/vbatts/docker/compare/vbatts-gh6385

@crosbymichael
Copy link
Contributor

LGTM

@vbatts
Copy link
Contributor

vbatts commented Jun 17, 2014

@crosbymichael but the json is still not the same. It should just do a copy of the file from the image graph.

@vieux
Copy link
Contributor Author

vieux commented Jun 17, 2014

@vbatts the only change is the create/mod/access time of the json file, not the content. So I don't see the issue.

@vbatts
Copy link
Contributor

vbatts commented Jun 19, 2014

@vieux I can test this PR again, but #6461 (comment) has the output of the json on disk and the one from docker save, and they are different ...

@vieux
Copy link
Contributor Author

vieux commented Jun 19, 2014

@vbatts can you provide me a failing test ?
My test compares the content, and it's the same.

@vbatts
Copy link
Contributor

vbatts commented Jun 19, 2014

@vieux i will work on getting a test. Your test's look good, but there are other use-cases in the life of an image to account for, and not affecting the layer's TarSum calculation (for the docker client or registry)

@vbatts
Copy link
Contributor

vbatts commented Jun 20, 2014

@vieux here is a branch that uses this branch of yours, and expressly does a raw json for docker save https://github.com/vbatts/docker/compare/vbatts-raw_json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker save: does not preserve the "json" docker load clears DockerVersion
3 participants