Skip to content

Expand chown value of COPY command#926

Merged
tonistiigi merged 2 commits intomoby:masterfrom
m-hu:35018-expandchown
Apr 16, 2019
Merged

Expand chown value of COPY command#926
tonistiigi merged 2 commits intomoby:masterfrom
m-hu:35018-expandchown

Conversation

@m-hu
Copy link
Copy Markdown
Contributor

@m-hu m-hu commented Apr 6, 2019

Signed-off-by: Hao Hu hao.hu.fr@gmail.com

for fixing the issue moby/moby#35018

Signed-off-by: Hao Hu <hao.hu.fr@gmail.com>
@tonistiigi
Copy link
Copy Markdown
Member

@AkihiroSuda @thaJeztah What's you thoughts on this? I think I'm ok with it. I guess we need to bump the version to 1.1 for this cause doesn't make sense to put it in experimental. We should find a solution for #815 as well in the same release cycle.

@pptime Could you add an integration test as well please.

Signed-off-by: Hao Hu <hao.hu.fr@gmail.com>
@m-hu
Copy link
Copy Markdown
Contributor Author

m-hu commented Apr 6, 2019

@tonistiigi some existing integration tests have been extended.

However, get the following tests failure

--- FAIL: TestIntegration/TestImportExportReproducibleIDs/worker=containerd-1.0/frontend=builtin (2.05s)
    require.go:157: 
        	Error Trace:	dockerfile_test.go:3475
        	            				run.go:172
        	Error:      	Not equal: 
        	            	expected: v1.Descriptor{MediaType:"application/vnd.docker.distribution.manifest.v2+json", Digest:"sha256:72d6198c65fcfd08077ca2cff75e7380d05c0fd27ced11db46d519e4e4144d37", Size:941, URLs:[]string(nil), Annotations:map[string]string(nil), Platform:(*v1.Platform)(nil)}
        	            	actual  : v1.Descriptor{MediaType:"application/vnd.docker.distribution.manifest.v2+json", Digest:"sha256:1f4a5d59899ffacdd010bdb3fd0c5416673ea866dbaab5a7f7fbeafaf0dfc232", Size:941, URLs:[]string(nil), Annotations:map[string]string(nil), Platform:(*v1.Platform)(nil)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,3 +2,3 @@
        	            	  MediaType: (string) (len=52) "application/vnd.docker.distribution.manifest.v2+json",
        	            	- Digest: (digest.Digest) (len=71) sha256:72d6198c65fcfd08077ca2cff75e7380d05c0fd27ced11db46d519e4e4144d37,
        	            	+ Digest: (digest.Digest) (len=71) sha256:1f4a5d59899ffacdd010bdb3fd0c5416673ea866dbaab5a7f7fbeafaf0dfc232,
        	            	  Size: (int64) 941,
        	Test:       	TestIntegration/TestImportExportReproducibleIDs/worker=containerd-1.0/frontend=builtin
    oci.go:123: stderr: /usr/bin/buildkitd

It seems to be the same issue as in #909. Is it a really flaky?

@thaJeztah
Copy link
Copy Markdown
Member

Functionally, I'm ok with this, but I recall that you had concerns about caching and which scope to use for expansion of ARG (global vs local?)

@m-hu
Copy link
Copy Markdown
Contributor Author

m-hu commented Apr 7, 2019 via email

@tonistiigi
Copy link
Copy Markdown
Member

@thaJeztah That was for --from. In here it should use local scope.

@thaJeztah
Copy link
Copy Markdown
Member

Ah, yes; no objections then

@m-hu
Copy link
Copy Markdown
Contributor Author

m-hu commented Apr 8, 2019

all right, still some arbitrary fails...but it is pointing to some interesting stuff anyway

@tonistiigi
Copy link
Copy Markdown
Member

ping @AkihiroSuda @tiborvass

@darkdragon-001
Copy link
Copy Markdown

darkdragon-001 commented Jun 20, 2019

Will this also be backported to Docker 18.09 or when can we see this in an official release?

@thaJeztah
Copy link
Copy Markdown
Member

This will be in the next (19.03) release of docker; https://github.com/docker/engine/blob/19.03/vendor/github.com/moby/buildkit/frontend/dockerfile/instructions/commands.go#L203

@bayandin
Copy link
Copy Markdown

bayandin commented Jul 3, 2019

@thaJeztah That was for --from. In here it should use local scope.

@thaJeztah @tonistiigi hey guys, I'm wondering about expanding --from for COPY, it seems you've already had a discussion regarding it, and there could be some problems? Could you elaborate a bit more?

colinhoglund added a commit to kanopy-platform/drone-bazelisk-ecr that referenced this pull request Apr 8, 2020
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.

5 participants