Skip to content

Use spf13/cobra for docker network and subcommands#23210

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vdemeester:migrate-network-command-to-cobra
Jun 6, 2016
Merged

Use spf13/cobra for docker network and subcommands#23210
thaJeztah merged 1 commit intomoby:masterfrom
vdemeester:migrate-network-command-to-cobra

Conversation

@vdemeester
Copy link
Copy Markdown
Member

@vdemeester vdemeester commented Jun 2, 2016 •
edited
Loading

  • Migrates network command and subcommands (connect, create, disconnect, inspect, list and remove) to spf13/cobra 🐍.
  • Create a RequiredExactArgs helper function for command that require an exact number of arguments.

/cc @dnephin @thaJeztah @LK4D4

🐸

Signed-off-by: Vincent Demeester [email protected]

@vdemeester vdemeester added this to the 1.12.0 milestone Jun 2, 2016
@vdemeester vdemeester changed the title Migrate network command to cobra Migrate network commands to cobra Jun 2, 2016
@vdemeester vdemeester added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 2, 2016
@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch from c90d150 to 7b07d4c Compare June 2, 2016 16:56
dnephin
dnephin reviewed Jun 2, 2016
View reviewed changes
Comment thread api/client/network/connect.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Jun 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be better to continue to use opts.NewListOpts(runconfigopts.ValidateLink) for this. It's nice when all the flag/arg validation happens at the same spot, so the error messages are consistent (you get the usage message). You can use it with flags.Var(&opts.links, ...) by making the type opts.ListOpts, and setting the default value when you declare opts.

With this setup I don't think you'll get the usage info.

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Jun 2, 2016

Choose a reason for hiding this comment

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

ah yes 👍

@dnephin dnephin mentioned this pull request Jun 2, 2016
Closed
43 tasks
dnephin
dnephin reviewed Jun 2, 2016
View reviewed changes
Comment thread cli/required.go Outdated
Copy link
Copy Markdown
Member

@dnephin dnephin Jun 2, 2016

Choose a reason for hiding this comment

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

Nice! I was thinking maybe ExactArgs(number int), what do you think?

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Jun 2, 2016

Choose a reason for hiding this comment

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

Sounds better, I'll update 😉

@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch from 7b07d4c to 28d5815 Compare June 3, 2016 09:20
@vdemeester vdemeester removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 3, 2016
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 3, 2016

built and tried this, and all looks to be working correctly 👍

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 3, 2016

ping @dnephin PTAL, its green 💚

@dnephin
Copy link
Copy Markdown
Member

dnephin commented Jun 3, 2016

LGTM!

@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch from 28d5815 to 2ebb612 Compare June 4, 2016 10:31
@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Jun 4, 2016

Rebased 👼

@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch from 2ebb612 to 47d39d6 Compare June 4, 2016 17:08
@vdemeester vdemeester changed the title Migrate network commands to cobra Use spf13/cobra for docker network and subcommands Jun 5, 2016
@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch 2 times, most recently from 22232f8 to e833b3c Compare June 5, 2016 20:42
@vdemeester
Migrate network command to cobra
4bd202b 
- Migrates network command and subcommands (connect, create, disconnect,
  inspect, list and remove) to spf13/cobra
- Create a RequiredExactArgs helper function for command that require an
  exact number of arguments.

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the migrate-network-command-to-cobra branch from e833b3c to 4bd202b Compare June 6, 2016 08:29
@thaJeztah thaJeztah mentioned this pull request Jun 6, 2016
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 6, 2016

LGTM

Janky is green, but didn't notify GitHub; https://jenkins.dockerproject.org/job/Docker-PRs/28337/console

docker-pr28337
09:37:46 Archiving artifacts
09:37:47 Notifying endpoint 'HTTP:https://leeroy.dockerproject.org/notification/jenkins'
09:37:49 [WS-CLEANUP] Deleting project workspace...[WS-CLEANUP] done
09:37:49 Finished: SUCCESS

@thaJeztah thaJeztah merged commit 07a7c06 into moby:master Jun 6, 2016
@vdemeester vdemeester deleted the migrate-network-command-to-cobra branch June 6, 2016 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/cli Client status/2-code-review

Projects

None yet

Milestone

1.12.0

Development

Successfully merging this pull request may close these issues.

4 participants

@vdemeester @thaJeztah @dnephin @GordonTheTurtle