-
Notifications
You must be signed in to change notification settings - Fork 576
enable experimental commands by default, and remove BUILDX_EXPERIMENTAL
env-var
#3241
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
base: master
Are you sure you want to change the base?
Conversation
7d2a56d
to
2298179
Compare
@@ -84,10 +84,6 @@ func NewRootCmd(name string, isPlugin bool, dockerCli *command.DockerCli) *cobra | |||
"using default config store", | |||
)) | |||
|
|||
if !confutil.IsExperimental() { |
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.
Also considering if we should just (as we did for the CLI) always enable the debug
command;
Lines 121 to 123 in 08dd378
if confutil.IsExperimental() { | |
cmd.AddCommand(debugCmd(dockerCli, opts)) | |
} |
The output already shows it's experimental;
BUILDX_EXPERIMENTAL=1 docker buildx debug --help
Usage: docker buildx debug [OPTIONS] COMMAND
Start debugger (EXPERIMENTAL)
EXPERIMENTAL:
docker buildx debug is an experimental feature.
Experimental features provide early access to product functionality. These
features may change between releases without warning, or can be removed from a
future release. Learn more about experimental features in our documentation:
https://docs.docker.com/go/experimental/
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.
Also considering if we should just (as we did for the CLI) always enable the
debug
command
With on-going work with debug
command related to new DAP implementation I think we should keep it experimental for now:
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.
Yeah, I was considering that we would have it visible, but call out that it's experimental? It may help getting more people to try it if we don't require config changes, as long as we explicitly call out that it's experimental?
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.
In the future I agree that we should deprecate BUILDX_EXPERIMENTAL
as the CLI already marks commands as experimental 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.
For what it's worth, the debug adapter stuff is going under another command in dap
. Right now, debug
controls the monitor. We do potentially have plans to completely remove the debug
command and integrate it directly with build.
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.
Discussed with @tonistiigi, we would need to always enable the debug
command if we are removing this hint from help so users are aware of it.
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
It would also solve #2495 that I wanted to fix today and remembered your PR 😅
We should get this one in.
2298179
to
f409f61
Compare
Did a quick rebase to get a fresh run of CI and moved it out of draft |
All green again; PTAL |
This was added in 02c2073, which changed the buildx CLI to match the behavior of the docker CLI, which conditionally hid some options that were marked experimental. The CLI now shows has client-side experimental options enabled by default (but labeled as experimental where suitable). Before this patch, every command would print this message unless experimental was enabled; docker buildx --help ... Run 'docker buildx COMMAND --help' for more information on a command. Experimental commands and flags are hidden. Set BUILDX_EXPERIMENTAL=1 to show them. With this patch applied, that message is no longer shown. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Experimental commands are labeled as experimental, but we can enable them by default to encourage users to try them. Signed-off-by: Sebastiaan van Stijn <[email protected]>
As the experimental commands are enabled by default now, there's no need to set this env-var for generating the docs. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This function is no longer used, and does not have external consumers; https://grep.app/search?f.lang=Go&q=.IsExperimental%28%29 Signed-off-by: Sebastiaan van Stijn <[email protected]>
t.TempDir() already cleans up after the test. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Experimental commands are now enabled by default, so there should not be a need to run tests separately. Signed-off-by: Sebastiaan van Stijn <[email protected]>
It was only used to illustrate the concept, so let's use something generic. Signed-off-by: Sebastiaan van Stijn <[email protected]>
f409f61
to
484ab1b
Compare
BUILDX_EXPERIMENTAL
env-var
Updated to now enable experimental commands by default;
|
t.Cleanup(func() { | ||
os.RemoveAll(destDir) | ||
}) |
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.
Not really related, but my IDE was complaining about the unhandled error, and it was redundant, so added a commit to remove it 😂
@thaJeztah For a follow-up I was wondering if we still need to alter buildx/util/cobrautil/cobrautil.go Lines 22 to 53 in 5569825
Maybe docker/cli has this kind of helpers already? |
docker/cli adds an experimental warning to the "usage" / "help" output of the command itself, but does not add a We should probably look what we want to do there; to some extend the "ALL CAPS" format seems a bit aggressive, but maybe there's some middle ground. |
I saw #3240 which reminded me that I wanted to open this patch as suggestion.
remove "experimental" hint from help / usage output
This was added in 02c2073, which changed the buildx CLI to match the behavior of the docker CLI, which conditionally hid some options that were marked experimental. The CLI now shows has client-side experimental options enabled by default (but labeled as experimental where suitable).
Before this patch, every command would print this message unless experimental was enabled;
With this patch applied, that message is no longer shown.
commands: enable experimental commands by default
Experimental commands are labeled as experimental, but we can enable
them by default to encourage users to try them.
remove uses of BUILDX_EXPERIMENTAL for generating docs
As the experimental commands are enabled by default now, there's
no need to set this env-var for generating the docs.
util/confutil: remove IsExperimental() utility
This function is no longer used, and does not have external
consumers; https://grep.app/search?f.lang=Go&q=.IsExperimental%28%29
tests: remove redundant cleanup
t.TempDir() already cleans up after the test.
remove "experimental" integration tests
Experimental commands are now enabled by default, so there should not
be a need to run tests separately.
docs: remove use of BUILDX_EXPERIMENTAL as example
It was only used to illustrate the concept, so let's use something
generic.