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

driver: make buildkitd "config" and "flags" names consistent #2268

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Feb 21, 2024

I found it confusing to have config as driver opt instead of buildkitd-config so we are consistent with buildkitd-flags. Same for variables not aligned across our code base.

This is also relevant for #2270

@crazy-max
Copy link
Member Author

ci failure for docs-upstream workflow is not related: #2269

Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

General: should it be --buildkitd-x or --buildkit-x ?

I don't think we would expect buildx to refer to buildkit as anything other than a daemon. So maybe the d can be removed. I think it would make it easier to remember.

docs/reference/buildx_create.md Outdated Show resolved Hide resolved
docs/reference/buildx_create.md Outdated Show resolved Hide resolved
builder/builder.go Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

crazy-max commented Feb 21, 2024

General: should it be --buildkitd-x or --buildkit-x ?

I don't think we would expect buildx to refer to buildkit as anything other than a daemon. So maybe the d can be removed. I think it would make it easier to remember.

Hum I think buildkitd is the correct assumption as we are passing them to the daemon. Would also avoid deprecating buildkitd-flags but I agree for casual users buildkit is well-known. On the other hand, someone setting these flags knows what he's doing.

build/build.go Outdated Show resolved Hide resolved
build/build.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Should we keep the old flags as hidden / deprecated for 1 .. 2 releases so that we can print a meaningful error if someone is using these? (didn't fully read through the diff, so perhaps we have something for that).

@crazy-max
Copy link
Member Author

crazy-max commented Feb 22, 2024

Should we keep the old flags as hidden / deprecated for 1 .. 2 releases so that we can print a meaningful error if someone is using these? (didn't fully read through the diff, so perhaps we have something for that).

@thaJeztah Yes we keep config as hidden: https:/docker/buildx/pull/2268/files#diff-e163527133bf99ae0eee4eacce5a81fd349ab589b9e85c205cba4618c5562305R113-R116

This is similar to https:/docker/cli/blob/1443014cbd604b460548a7729ec8ec510a5dbc5d/cli/command/container/opts.go#L247-L250

@thaJeztah
Copy link
Member

Ah, thanks! Are we planning to officially deprecate the old one? (in that case, we could consider printing a deprecation warning when used, which can be "non-fatal" for now, but made "fatal" in a future release).

@crazy-max
Copy link
Member Author

Ah, thanks! Are we planning to officially deprecate the old one? (in that case, we could consider printing a deprecation warning when used, which can be "non-fatal" for now, but made "fatal" in a future release).

Fine to keep it imo and I don't think the cli prints such warning atm for docker run --net-alias ... for example?

Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

LGTM

@tonistiigi tonistiigi merged commit d8e9c7f into docker:master Feb 22, 2024
65 checks passed
@crazy-max crazy-max deleted the dirver-align-flags branch February 23, 2024 10:04
@crazy-max
Copy link
Member Author

Needs follow-up on https:/crazy-max/docker-setup-buildx-action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants