-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
dockerfile: add --chmod support for COPY/ADD command #1492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add integration tests for this https://github.com/moby/buildkit/blob/master/frontend/dockerfile/dockerfile_test.go
@AkihiroSuda @tiborvass @thaJeztah Do we take this directly to stable channel or should it go through experimental?
if useFileOp(opt.buildArgValues, opt.llbCaps) { | ||
return dispatchCopyFileOp(d, c, sourceState, isAddCommand, cmdToPrint, chown, opt) | ||
return dispatchCopyFileOp(d, c, sourceState, isAddCommand, cmdToPrint, chown, chmod, opt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is only supported in fileop it should error if it is not supported in the llbcaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll add the it test
3953cc1
to
b45e7ca
Compare
Seems safe for stable channel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something wrong with the commits. Merge commits are not allowed in PRs
return dispatchCopyFileOp(d, c, sourceState, isAddCommand, cmdToPrint, chown, chmod, loc, opt) | ||
} | ||
|
||
if chmod != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if caps != nil && caps.Supports(pb.CapFileBase) != nil {
return errorss.Wrap(caps.Supports(pb.CapFileBase), "chmod is not supported")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
@heychenbin ping |
Sorry, I did not see the notify message, I'll amend it. Because of the network problem, I can not run integration test right now. I will test it and re-push it when I come back home. Sorry for replying so late. |
05067c4
to
9547c88
Compare
Signed-off-by: Chenbin <[email protected]>
Perhaps it would be good to have this in experimental first 🤔. I recall there was some discussion on the moby issue (moby/moby#34819) related to "how will we support this for Windows images". I understand BuildKit doesn't have Windows support, so not a real issue (yet); but having some plan on how to evolve in that direction may be good (where "plan" could also mean: won't be supported) Another thing to look at is (and we should look at that as well for the existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this being in the stable channel. Don't see how we could remove it.
if opt.llbCaps != nil && opt.llbCaps.Supports(pb.CapFileBase) != nil { | ||
return errors.Wrap(opt.llbCaps.Supports(pb.CapFileBase), "chmod is not supported") | ||
} | ||
return errors.New("chmod is not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If chmod != ""
it always errors? I think this part needs to be removed or refactored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hinshun only on old buildkit instances that don't have fileop support. Otherwise, it is an early return on line 836.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, so if BuildKit doesn't have fileop and chmod is specified, it should still return error... a bit confusing. My bad.
I agree that this would be a bad design. Has it been addressed at all? |
This is not quite correct. The real behavior is that We can just make it clear in docs that this is the expected behavior. I don't see a reason to not allow a behavior change in here as well. It's not very likely that people today are already using the flags on archive inputs but expect the current behavior (although there is a case with multiple inputs). But atm, for the |
Great addition! Which is the first version that includes this? |
@tonistiigi Do you know in which release this was included? I could not find it in the release notes of 2.10.0 |
@thernstig It is in 20.10 (cc @tiborvass ) |
Thanks @tonistiigi - hope the release notes can get an update. It's quite a great feature. |
@Chenbinx @tonistiigi I tried this, but it seems this does not work : COPY --chmod=g=u --from=builder /server/ /server/
Is there no way to achieve this with the new |
@thernstig I opened docker/cli#2941 to get this documented. |
- add support for the --chmod flag for `ADD` - add support for the --chmod flag for `COPY` - add tests for the --chmod flag Buildkit supports setting a new mode on a file during `COPY`/`ADD` instructions with the --chmod flag. See: moby/buildkit#1492 fixes: hadolint/hadolint#609
- add support for the --chmod flag for `ADD` - add support for the --chmod flag for `COPY` - add tests for the --chmod flag - add tests for pretty printer Buildkit supports setting a new mode on a file during `COPY`/`ADD` instructions with the --chmod flag. See: moby/buildkit#1492 fixes: hadolint/hadolint#609
I found that llb.CopyInfo already support mode option, but I'm not sure why there is no support for --chmod in COPY/ADD command. So I made little change to support it.
By the way, --chmod is really useful for us to make a smaller docker image in a easy way.
Signed-off-by: Chen Bin [email protected]