-
Notifications
You must be signed in to change notification settings - Fork 253
Conversation
Signed-off-by: aiordache <[email protected]>
cli/cmd/compose/build.go
Outdated
cmd.Flags().BoolVar(&opts.noCache, "no-cache", false, "Do not use cache when building the image") | ||
cmd.Flags().Bool("no-rm", false, "Do not remove intermediate containers after a successful build. DEPRECATED") | ||
cmd.Flags().MarkHidden("no-rm") //nolint:errcheck | ||
cmd.Flags().StringVarP(&opts.memory, "memory", "m", "", "Set memory limit for the build container. DEPRECATED") |
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.
AFAIK this hasn't been deprecated (why would it?)
docker-compose 1.28.5 says -m, --memory MEM Set memory limit for the build container.
- no deprecation notice here
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 don't think they're deprecated, but some of these are not (yet) supported on BuildKit; there's a list of options that are not available when using buildkit here; moby/moby#40379
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.
right, so we should better show a warning explaining this option is not (yet) implemented and will be ignored.
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.
On the docker cli, we conditionally hide some of these if buildkit is used as builder (look for the no-buildkit
annotation for these flags; https://github.com/docker/cli/blob/e3d93058fdbb7ecd24aa68ca710908e075a0f2ed/cli/command/image/build.go#L123-L124
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.
right, so we should better show a warning explaining this option is not (yet) implemented and will be ignored.
Yes, I had a PR in the cli, that would produce an error, but possibly a "warning" would be better; I was waiting for @tonistiigi to answer he agreed on that (instead of error); docker/cli#2736
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.
while @aiordache is on PTO, I added a warning if the --memory
flag is set, and it's just ignored. Removed DEPRECATED
from the description (but it's hidden anyway)
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.
LGTM but need clarification on deprecating --memory (which seems to me a usefull one)
…rning and ignore flag Signed-off-by: Guillaume Tardif <[email protected]>
Added remaining build flags. Only one applied is the
no-cache
one.With
--no-cache
Closes #1468