Use spf13/cobra for docker update by yongtang · Pull Request #23281 · moby/moby

yongtang · 2016-06-05T21:05:00Z

This fix is part of the effort to convert commands to spf13/cobra #23211.

Thif fix coverted command docker update to use spf13/cobra

Signed-off-by: Yong Tang [email protected]

cpuguy83 · 2016-06-06T01:10:07Z

LGTM

thaJeztah · 2016-06-06T11:54:54Z

sorry @yongtang, this one needs a rebase as well 😢

yongtang · 2016-06-06T12:18:56Z

Thanks @thaJeztah. The PR has been rebased.

vdemeester · 2016-06-06T14:05:57Z

I feel the pointer should stay the same 👼

Thanks @vdemeester. Are you referring to changing the fields of updateOptions to pointers and always prefix with fl (like old version) as below?

type updateOptions struct { flblkioWeight *uint16 flCPUPeriod *int64 flCPUQuota *int64 ... }

on the Resources struct 👼

Change

Memory: memory,

to

Memory: flMemory,

?

CPUShares: *opts.cpuShares,

I see. Let me update the pull request.

Thanks @vdemeester the pointers has been changed and PR has been updated. Let me know if there are any issues.

icecrime · 2016-06-06T17:54:00Z

What the?? On gccgo:

06:25:33 FAIL: docker_cli_help_test.go:15: TestHelpTextVerify.pN52_github_com_docker_docker_integration_cli.DockerSuite
06:25:33 
06:25:33 docker_cli_help_test.go:143:
06:25:33     c.Fatal(err)
06:25:33 ... Error: Error on "logout" help. non-empty stderr:"Aborted\n\npanic during panic\n\nruntime stack:\nstack trace unavailable\n"

yongtang · 2016-06-07T02:13:28Z

Thanks @icecrime for the review. I am hoping the failure is a one-time failure but I will keep an eye on it in case it is repeatable.

thaJeztah · 2016-06-07T12:30:10Z

ping @vdemeester PTAL

vdemeester · 2016-06-07T13:54:28Z

I don't think pointer are needed there, just in the resource struct 👼

Thanks @vdemeester but if we don't use pointers like

blkioWeight *uint16 ... cpuShares *int64

here, we may not be able to reference pointers in resource

BlkioWeight: *opts.blkioWeight, .... CPUShares: *opts.cpuShares,

as well?

Maybe I missed something?

@vdemeester ping :)

thaJeztah · 2016-06-14T08:22:58Z

Sorry @yongtang, this needs a rebase now 😢

yongtang · 2016-06-14T13:21:34Z

Thanks @thaJeztah the pull request has been rebased.

yongtang · 2016-06-14T20:48:33Z

Ping @vdemeester @thaJeztah.

LK4D4 · 2016-06-22T16:23:34Z

Use flags.Uint16Var to avoid pointers. Take api/client/container/run.go as reference. Thanks!

LK4D4 · 2016-06-22T16:28:08Z

@yongtang Sorry for delay! Let me know if you want us to carry PR.

yongtang · 2016-06-22T17:00:22Z

@LK4D4 Thanks for the review. I updated the pull request to change the blkioWeight in opts. Please let me know if there are any issues.

LK4D4 · 2016-06-22T17:20:29Z

Please use Var stuff for all options.

Thanks @LK4D4 let me update the PR shortly.

This fix is part of the effort to convert commands to spf13/cobra moby#23211. Thif fix coverted command `docker update` to use spf13/cobra Signed-off-by: Yong Tang <[email protected]>

yongtang · 2016-06-22T18:28:24Z

Thanks @LK4D4 The pull request has been updated. Please let me know if there are any other issues.

LK4D4 · 2016-06-22T18:30:25Z

@yongtang perfect! I'll merge if ci pass.

vdemeester · 2016-06-22T18:30:35Z

LGTM 🐮 assuming it's green 😇

GordonTheTurtle added the status/0-triage label Jun 5, 2016

thaJeztah added area/api API status/2-code-review and removed status/0-triage labels Jun 5, 2016

thaJeztah added this to the 1.12.0 milestone Jun 5, 2016

dnephin mentioned this pull request Jun 5, 2016

Convert commands to spf13/cobra #23211

Closed

43 tasks

yongtang force-pushed the 23211-spf13-cobra-update branch from 596ab91 to e5a132e Compare June 6, 2016 00:31

yongtang force-pushed the 23211-spf13-cobra-update branch from e5a132e to ddc8984 Compare June 6, 2016 12:18

vdemeester reviewed Jun 6, 2016
View reviewed changes

yongtang force-pushed the 23211-spf13-cobra-update branch from ddc8984 to da925d5 Compare June 7, 2016 02:01

vdemeester reviewed Jun 7, 2016
View reviewed changes

yongtang force-pushed the 23211-spf13-cobra-update branch 2 times, most recently from 4f9cddd to 4d7d52a Compare June 11, 2016 21:27

icecrime modified the milestone: 1.12.0 Jun 13, 2016

yongtang force-pushed the 23211-spf13-cobra-update branch from 4d7d52a to 9ba56aa Compare June 14, 2016 02:07

yongtang force-pushed the 23211-spf13-cobra-update branch from 9ba56aa to ea58396 Compare June 14, 2016 13:21

LK4D4 reviewed Jun 22, 2016
View reviewed changes

yongtang force-pushed the 23211-spf13-cobra-update branch from ea58396 to 28ba5e7 Compare June 22, 2016 16:58

LK4D4 reviewed Jun 22, 2016
View reviewed changes

Use spf13/cobra for docker update

9765593

This fix is part of the effort to convert commands to spf13/cobra moby#23211. Thif fix coverted command `docker update` to use spf13/cobra Signed-off-by: Yong Tang <[email protected]>

yongtang force-pushed the 23211-spf13-cobra-update branch from 28ba5e7 to 9765593 Compare June 22, 2016 18:07

cpuguy83 added status/4-merge and removed status/2-code-review labels Jun 22, 2016

dnephin added this to the 1.13.0 milestone Jun 22, 2016

thaJeztah merged commit e5a8a77 into moby:master Jun 24, 2016

yongtang deleted the 23211-spf13-cobra-update branch June 25, 2016 12:31

Conversation

yongtang commented Jun 5, 2016

Uh oh!

cpuguy83 commented Jun 6, 2016

Uh oh!

thaJeztah commented Jun 6, 2016

Uh oh!

yongtang commented Jun 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented Jun 6, 2016

Uh oh!

yongtang commented Jun 7, 2016

Uh oh!

thaJeztah commented Jun 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 14, 2016

Uh oh!

yongtang commented Jun 14, 2016

Uh oh!

yongtang commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jun 22, 2016

Uh oh!

yongtang commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Jun 22, 2016

Uh oh!

LK4D4 commented Jun 22, 2016

Uh oh!

vdemeester commented Jun 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants