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

build: warning msg on deprecated flags #810

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Oct 20, 2021

Relates to moby/moby#40379

Add warning message for deprecated flags via pflag annotations:

$ docker buildx build --load --squash --cgroup-parent /build .
WARN cgroup-parent is not implemented.
WARN squash flag was never taken out of experimental and is deprecated with BuildKit. You should squash inside build using the multi-stage feature for efficiency.
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 2.94kB 0.1s done
#1 DONE 0.1s
  • logrus formatter has been changed to avoid prefix like WARN[000].
  • long description has been added to the root command.

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash/Security-opt/isolation should be deprecated. Replacement for squash is to squash inside build (eg. in Dockerfile with additional stage). For Security-opt, RUN --security=insecure should be used. For --net=custom alternative is to set it on buildx create. I think this one could remain error.

For cgroup-parent I'm not sure. Maybe let's just implement it although it is a bad design. (edit: maybe implement and leave it as hidden?)

From quick overview, the rest can remain hidden as they were. No point to show these messages for flags that don't change build outcome and when we are not sure if they will fit in design or not.

For formatting, no capitalized messages. The logrus default seems fine to me.

@crazy-max
Copy link
Member Author

Squash/Security-opt/isolation should be deprecated. Replacement for squash is to squash inside build (eg. in Dockerfile with additional stage). For Security-opt, RUN --security=insecure should be used. For --net=custom alternative is to set it on buildx create. I think this one could remain error.

Ok sgtm

For cgroup-parent I'm not sure. Maybe let's just implement it although it is a bad design. (edit: maybe implement and leave it as hidden?)

Yeah implement and keep it hidden lgtm.

From quick overview, the rest can remain hidden as they were. No point to show these messages for flags that don't change build outcome and when we are not sure if they will fit in design or not.

Done

For formatting, no capitalized messages.

Sure

The logrus default seems fine to me.

You mean with the default timestamp included so revert my changes?

@crazy-max crazy-max changed the title build: warning msg on deprecated or not implemented flags build: warning msg on deprecated flags Oct 21, 2021
@tonistiigi tonistiigi added this to the v0.7.0 milestone Oct 25, 2021
cmd/buildx/main.go Outdated Show resolved Hide resolved
commands/build.go Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved
commands/build.go Outdated Show resolved Hide resolved

flags.StringVar(&ignore, "cgroup-parent", "", "Optional parent cgroup for the container")
flags.MarkHidden("cgroup-parent")
flags.SetAnnotation("cgroup-parent", "flag-warn", []string{"cgroup-parent is not implemented."})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess plan was to implement this so set it in frontend-opt instead.

commands/build.go Outdated Show resolved Hide resolved
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the warn-flags-depre branch 2 times, most recently from 1e1d7fe to c12379d Compare October 26, 2021 15:30
build/build.go Outdated Show resolved Hide resolved
docs/reference/buildx_create.md Outdated Show resolved Hide resolved

```console
$ docker run -d --name jaeger -p 6831:6831/udp -p 16686:16686 jaegertracing/all-in-one
$ docker buildx create --name builder --driver docker-container --driver-opt network=host --driver-opt env.JAEGER_TRACE=localhost:6831 --use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of --driver-opt network=host we could use the jaeger container IP directly. Or maybe there is a parameter for jaeger to listen 6831 on gateway IP. This can be done later as well.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@tonistiigi tonistiigi merged commit 4690e14 into docker:master Oct 26, 2021
@crazy-max crazy-max deleted the warn-flags-depre branch October 26, 2021 23:29
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.

2 participants