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

Convert docker and dockerd commands to spf13/cobra #25354

Merged
merged 7 commits into from Aug 25, 2016

Conversation

dnephin
Copy link
Member

@dnephin dnephin commented Aug 3, 2016

Fixes #23211
Fixes #25547

  • removes pkg/mflag
  • removes most of cli/ and consolidates the rest of the package into fewer files

@tonistiigi
Copy link
Member

@dnephin Test failures seem related.

@tonistiigi tonistiigi added the status/failing-ci Indicates that the PR in its current state fails the test suite label Aug 4, 2016
@dnephin
Copy link
Member Author

dnephin commented Aug 4, 2016

I think they are related. Having some trouble finding the cause, but still working on it.

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 5, 2016
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 5, 2016
@dnephin dnephin force-pushed the remove-mflag-from-dockerd branch 2 times, most recently from 4a90e29 to 40c3a15 Compare August 9, 2016 19:27
@dnephin dnephin force-pushed the remove-mflag-from-dockerd branch 7 times, most recently from 8f09eac to 2b83d17 Compare August 17, 2016 15:44
@dnephin dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Aug 17, 2016
@dnephin
Copy link
Member Author

dnephin commented Aug 17, 2016

This is green!

@dnephin dnephin force-pushed the remove-mflag-from-dockerd branch 2 times, most recently from 76d3804 to 1357c1e Compare August 24, 2016 15:43
@@ -4,12 +4,22 @@ package main

import (
"fmt"
"github.com/spf13/cobra"
Copy link
Member

Choose a reason for hiding this comment

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

nit: this import is misplaced 👼

@vdemeester
Copy link
Member

LGTM 🐸

@tonistiigi
Copy link
Member

LGTM

Support command traversal

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix the daemon proxy for cobra commands.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Improve error messages raised by assert.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Cleanup cobra integration
Update windows files for cobra and pflags
Cleanup SetupRootcmd, and remove unnecessary SetFlagErrorFunc.
Use cobra command traversal

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Also consolidate the leftover packages under cli.
Remove pkg/mflag.
Make manpage generation work with new cobra layout.
Remove remaining mflag and fix tests after rebase with master.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Support args to RunCommand
Fix docker help text test.
Fix for ipv6 tests.
Fix TLSverify option.
Fix TestDaemonDiscoveryBackendConfigReload
Use tempfile for another test.
Restore missing flag.
Fix tests for removal of shlex.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@2opremio
Copy link

They'll find out pretty quickly when the files don't exist anymore

You bet! We just did.

@runcom
Copy link
Member

runcom commented Aug 26, 2016

This is breaking master, with this ExecStart (note using the compat docker daemon):

ExecStart=/usr/bin/docker daemon \
          --exec-opt native.cgroupdriver=systemd \
          $OPTIONS \
          $DOCKER_STORAGE_OPTIONS \
          $DOCKER_NETWORK_OPTIONS \
          $INSECURE_REGISTRY

I'm getting:

Aug 26 14:49:10 localhost.localdomain docker[25774]: unknown flag: --exec-opt
Aug 26 14:49:10 localhost.localdomain docker[25774]: See 'docker daemon --help'.

This has somehow broke the old docker daemon compat command

@dnephin

@runcom
Copy link
Member

runcom commented Aug 26, 2016

@dnephin can we also add a test which uses docker daemon to start up a daemon to not regress on this in the future? (at least till we support docker daemon)

@dnephin
Copy link
Member Author

dnephin commented Aug 26, 2016

@runcom ok, i'll take a look, and add the missing test. i thought we had one for that.

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.

None yet

7 participants