Skip to content

build: warning msg on deprecated flags#810

Merged
tonistiigi merged 3 commits intodocker:masterfrom
crazy-max:warn-flags-depre
Oct 26, 2021
Merged

build: warning msg on deprecated flags#810
tonistiigi merged 3 commits intodocker:masterfrom
crazy-max:warn-flags-depre

Conversation

@crazy-max
Copy link
Copy Markdown
Member

@crazy-max crazy-max commented Oct 20, 2021 •
edited
Loading

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

crazy-max
crazy-max commented Oct 20, 2021
View reviewed changes
tonistiigi
tonistiigi reviewed Oct 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment •
edited
Loading

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
Copy Markdown
Member Author

crazy-max commented Oct 21, 2021

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
tonistiigi
tonistiigi reviewed Oct 26, 2021
View reviewed changes
commands/build.go Outdated

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
Copy Markdown
Member

@tonistiigi tonistiigi Oct 26, 2021

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.

tonistiigi
tonistiigi reviewed Oct 26, 2021
View reviewed changes
tonistiigi
tonistiigi reviewed Oct 26, 2021
View reviewed changes
@crazy-max
add long description to root command
e150d7b 
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
refactor: flags lexical order
36a37a6 
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
tonistiigi
tonistiigi reviewed Oct 26, 2021
View reviewed changes
docs/reference/buildx_create.md

```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
Copy Markdown
Member

@tonistiigi tonistiigi Oct 26, 2021

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.

@crazy-max
build: warning on deprecated flags
25d2f73 
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
This was referenced Oct 27, 2021
Merged
Merged
@vvoland vvoland mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.7.0

Development

Successfully merging this pull request may close these issues.

2 participants

@crazy-max @tonistiigi