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.

Sign up for GitHub

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

Jump to bottom

Deprecate "daemon" subcommand #26834

Merged
merged 1 commit into from
Sep 23, 2016
Merged

Deprecate "daemon" subcommand #26834

merged 1 commit into from
Sep 23, 2016

Conversation

thaJeztah
Copy link
Member

- What I did

- How I did it

- How to verify it

Build docker, and run docker daemon. Check that the output shows;

docker daemon
Command "daemon" is deprecated, and will be removed in docker 1.16. Please run `dockerd` directly.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

The daemon is in a separate (dockerd) binary
since docker 1.12, so should no longer be
used.

This marks the command as deprecated, and
adds it to the deprecated features list.

ping @jhowardmsft @dnephin PTAL

@thaJeztah thaJeztah added this to the 1.13.0 milestone Sep 22, 2016
thaJeztah
thaJeztah commented Sep 22, 2016
View reviewed changes
cmd/docker/daemon_unix.go
@@ -24,6 +24,7 @@ func newDaemonCommand() *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {
return runDaemon()
},
Deprecated: "and will be removed in docker 1.16. Please run `dockerd` directly.",
Copy link
Member Author

Choose a reason for hiding this comment

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

Cobra automatically prepends "Command "daemon" is deprecated, "

Copy link
Member

Choose a reason for hiding this comment

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

It's really pedantic, but shouldn't this be removed in Docker 1.16. (capital D)? 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, let me update ❤️

@thaJeztah thaJeztah mentioned this pull request Sep 22, 2016
Closed
@lowenna
Copy link
Member

lowenna commented Sep 22, 2016

Nice! LGTM. (And thanks, this was on my list to address later this week)

@dnephin
Copy link
Member

dnephin commented Sep 22, 2016

LGTM

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 22, 2016
@thaJeztah
Copy link
Member Author

CI is failing (I suspected that could happen, but was lazy and didn't run locally 😇 )

21:04:46 ----------------------------------------------------------------------
21:04:46 FAIL: docker_cli_help_test.go:17: DockerSuite.TestHelpTextVerify
21:04:46 
21:04:46 docker_cli_help_test.go:146:
21:04:46     c.Fatal(err)
21:04:46 ... Error: 
21:04:46 Command: /go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker daemon badArg
21:04:46 ExitCode: 1, Error: exit status 1
21:04:46 Stdout: Command "daemon" is deprecated, and will be removed in docker 1.16. Please run `dockerd` directly.
21:04:46 
21:04:46 Stderr: "dockerd" accepts no argument(s).
21:04:46 See 'dockerd --help'.
21:04:46 
21:04:46 Usage:  dockerd [OPTIONS]
21:04:46 
21:04:46 A self-sufficient runtime for containers.
21:04:46 
21:04:46 
21:04:46 Failures:
21:04:46 Expected stdout to contain "<NOTHING>"
21:04:46 
21:04:46 
21:04:46 
21:04:46 ----------------------------------------------------------------------

@thaJeztah
Deprecate "daemon" subcommand
bf58dd8 
The daemon is in a separate (dockerd) binary
since docker 1.12, so should no longer be
used.

This marks the command as deprecated, and
adds it to the deprecated features list.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
thaJeztah commented Sep 22, 2016
View reviewed changes
integration-cli/docker_cli_help_test.go
// Skip first line
helpOut = helpOut[1:]
}
// Skip first line, it is just "Commands:"
Copy link
Member Author

Choose a reason for hiding this comment

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

I first wanted to ignore "deprecation" message in the error handling, but decided that it's probably ok to just skip the daemon command in this test, as it's now deprecated.

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Sep 22, 2016
@thaJeztah
Copy link
Member Author

Updated the test, fingers crossed 😄

@vdemeester
Copy link
Member

LGTM 🐸

@estesp
Copy link
Contributor

estesp commented Sep 23, 2016

LGTM

@estesp estesp merged commit 57f0164 into moby:master Sep 23, 2016
@thaJeztah thaJeztah deleted the deprecate-docker-daemon branch September 23, 2016 12:54
@albers
Copy link
Member

albers commented Nov 5, 2016

@thaJeztah WDYT, should I remove completion for docker daemon? I usually remove completion of deprecated features, but this time I'm not sure.

@justincormack
Copy link
Contributor

I would remove the completion. Normally the daemon will be running anyway.

On 5 Nov 2016 17:58, "Harald Albers" notifications@github.com wrote:

@thaJeztah https://github.com/thaJeztah WDYT, should I remove
completion for docker daemon? I usually remove completion of deprecated
features, but this time I'm not sure.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#26834 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAdcPO8bn88r4FkkzEwFWI5u9PvYOu_rks5q7MPHgaJpZM4KEV2D
.

@albers albers mentioned this pull request Dec 19, 2016
Merged
@tuxity tuxity mentioned this pull request Jan 27, 2017
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 13, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tianon tianon tianon left review comments

Assignees
No one assigned
Labels
impact/changelog impact/cli impact/deprecation status/2-code-review
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.
9 participants
@thaJeztah @lowenna @dnephin @vdemeester @estesp @albers @justincormack @tianon @GordonTheTurtle