Fix #544: Avoiding chown to reduce the image size #1301
Conversation
There was a problem hiding this comment.
Looks good to me (except with some minor change request), but I wonder whether we should do an API detection like elsewhere, because --chown might not be available for every API version.
However, if --chown has been added since a long time (>1-2 years), then I think we could just do it unconditionally. But then we should mark this clearly in the changelog.
| for (CopyEntry entry : copyEntries) { | ||
| String dest = (basedir.equals("/") ? "" : basedir) + "/" + entry.destination; | ||
| DockerFileKeyword.ADD.addTo(b, " --chown=" | ||
| + (userParts.length > 2 ? |
There was a problem hiding this comment.
I'm asking because I don't see userPart[2] used anywhere.
There was a problem hiding this comment.
I think, for backwards compatibility we shoudl use userParts[2] to switch to that user after the ADD, like in the original code. (but we can mark that as deprecated, as its not really useful anymore IMO)
There was a problem hiding this comment.
Sorry, I might have missed this. Let me remove this and add a deprecation note to doc on next update
rohanKanojia
force-pushed
the
pr/issue544
branch
from
88174d8 to
2b65823
Compare
| if (userParts.length > 2) { | ||
| DockerFileKeyword.USER.addTo(b, "root"); | ||
| } | ||
| DockerFileKeyword.ADD.addTo(b, " --chown=" |
There was a problem hiding this comment.
Can't we just keep the COPY semnatics (instead of using ADD which works slightly different). I.e. I would add the extra command for chown to addCopyEntries and add an extra argument String ownerAndGroup to addCopyEntries() which can be either null or "user:group". That way we can preserve the COPY semantics.
i.e. use COPY --chown instead of ADD --chown.
There was a problem hiding this comment.
@rohanKanojia I'd like to add this to the release. Do you think there is a chance to get this in for the next release (i.e. change from ADD to COPY) ?
There was a problem hiding this comment.
ah, sorry. I was busy in last few weeks. Let me update this today
rohanKanojia
force-pushed
the
pr/issue544
branch
from
7363ce0 to
8a929e5
Compare
Fix #544