Hide experimental checkpoint features on Windows #1094
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ping @gbarr01 @jasonbivins PTAL |
|
For the documentation, this could be used in a similar way as the current "experimental", "experimentalCLI" and "API version" annotations are used;
|
thaJeztah
added
kind/experimental
status/2-code-review
and removed
status/0-triage
labels
thaJeztah
commented
May 31, 2018
docs/yaml/yaml.go
Outdated
| @@ -206,6 +211,9 @@ func genFlagResult(flags *pflag.FlagSet) []cmdOption { | |||
| if _, ok := flag.Annotations["swarm"]; ok { | |||
| opt.Swarm = true | |||
| } | |||
| if os, ok := flag.Annotations["ostype"]; ok && len(opt.OSType) == 0 && len(os) > 0 { | |||
| opt.OSType = os[0] | |||
There was a problem hiding this comment.
for the record; I decided to only use the first ostype here; theoretically, the annotation could have multiple, but we don't use that (and possibly never will); making this a single value is to make it consistent with the ostype on commands, and will make it easier to use for the docs team
@vdemeester let me know if you want me to add that as a comment or commit message
There was a problem hiding this comment.
A comment here or in the commit message seems safe(r) 👼 😛
silvin-lubecki
approved these changes
May 31, 2018
There was a problem hiding this comment.
LGTM!
By the way we should homogenize the error messages between flags and subcommands:
$ docker checkpoint rm --help
docker checkpoint rm is only supported on a Docker daemon running on linux, but the Docker daemon is running on windows
$ docker container start --checkpoint=foo mycontainer
"--checkpoint" requires the Docker daemon to run on linux, but the Docker daemon is running on windows
|
Let me update those errors as well to match |
This patch adds annotations to mark the checkpoint commands as Linux only, which
hides them if the daemon is running a non-matching operating-system type;
Before:
docker
Usage: docker COMMAND
A self-sufficient runtime for containers
...
Management Commands:
config Manage Docker configs
container Manage containers
image Manage images
After:
docker
Usage: docker COMMAND
A self-sufficient runtime for containers
...
Management Commands:
checkpoint Manage checkpoints
config Manage Docker configs
container Manage containers
image Manage images
This change also prints errors when attempting to use checkpoint commands or
flags if the feature is not supported by the Daemon's operating system;
$ docker checkpoint --help
docker checkpoint is only supported on a Docker daemon running on linux, but the Docker daemon is running on windows
$ docker checkpoint create --help
docker checkpoint create is only supported on a Docker daemon running on linux, but the Docker daemon is running on windows
$ docker checkpoint ls --help
docker checkpoint ls is only supported on a Docker daemon running on linux, but the Docker daemon is running on windows
$ docker checkpoint rm --help
docker checkpoint rm is only supported on a Docker daemon running on linux, but the Docker daemon is running on windows
$ docker container start --checkpoint=foo mycontainer
"--checkpoint" requires the Docker daemon to run on linux, but the Docker daemon is running on windows
$ docker container start --checkpoint-dir=/foo/bar mycontainer
"--checkpoint-dir" requires the Docker daemon to run on linux, but the Docker daemon is running on windows
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This patch adds an `os_type` property in the generated YAML docs, both for commands, and for flags; Note that the ostype annotation on flags can have multiple values set, however, multiple values are currently not used (and unlikely will). To simplify usage of the os_type property in the YAML, and for consistency with the same property for commands, we're only using the first ostype that's set. ```yaml command: docker checkpoint create short: Create a checkpoint from a running container long: Create a checkpoint from a running container usage: docker checkpoint create [OPTIONS] CONTAINER CHECKPOINT [flags] pname: docker checkpoint plink: docker_checkpoint.yaml options: - option: checkpoint-dir value_type: string description: Use a custom checkpoint storage directory deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false - option: leave-running value_type: bool default_value: "false" description: Leave the container running after checkpoint deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false deprecated: false min_api_version: "1.25" experimental: true experimentalcli: false kubernetes: false swarm: false os_type: windows ``` ```yaml command: docker container start short: Start one or more stopped containers long: Start one or more stopped containers usage: docker container start [OPTIONS] CONTAINER [CONTAINER...] [flags] pname: docker container plink: docker_container.yaml options: - option: attach shorthand: a value_type: bool default_value: "false" description: Attach STDOUT/STDERR and forward signals deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false - option: checkpoint value_type: string description: Restore from this checkpoint deprecated: false experimental: true experimentalcli: false kubernetes: false swarm: false os_type: linux - option: checkpoint-dir value_type: string description: Use a custom checkpoint storage directory deprecated: false experimental: true experimentalcli: false kubernetes: false swarm: false os_type: linux - option: detach-keys value_type: string description: Override the key sequence for detaching a container deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false - option: interactive shorthand: i value_type: bool default_value: "false" description: Attach container's STDIN deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false deprecated: false experimental: false experimentalcli: false kubernetes: false swarm: false ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
hide_checkpoint_on_windows
branch
from
791d7ea
to
be035a0
Compare
|
Updated, PTAL 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to docker/docs#6780
Mark checkpoint feature as Linux-only
This patch adds annotations to mark the checkpoint commands as Linux only, which
hides them if the daemon is running a non-matching operating-system type;
Before:
After:
This change also prints errors when attempting to use checkpoint commands or
flags if the feature is not supported by the Daemon's operating system;
YAML docs: add os_type property on flags and (sub)commands
This patch adds an
os_typeproperty in the generated YAML docs, both forcommands, and for flags;