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

Move duration opts into an opts package #77

Merged
merged 1 commit into from
May 18, 2017
Merged

Move duration opts into an opts package #77

merged 1 commit into from
May 18, 2017

Conversation

vdemeester
Copy link
Collaborator

They have nothing to do with service and could be used on their own.

🦁

– doing that because I wanted to re-use it without having to import/vendor service package.

Signed-off-by: Vincent Demeester vincent@sbr.pm

@mavenugo
Copy link
Contributor

@vdemeester should we consider moving the moby/opts package to docker/cli repo as part of the reorg effort ?

#62 is having a similar issue and @abhinandanpb decided to add the option as part of moby/opts to bring it back into docker/cli as part of the reorg. WDYT ?

@vdemeester
Copy link
Collaborator Author

@mavenugo yes, I think @dnephin opened an issue in moby/moby, moby/moby#33150. I'm assigning myself to it 😉

@dnephin
Copy link
Contributor

dnephin commented May 12, 2017

I agree with this change, but I think we should move in moby/moby/opts first, otherwise this is going to cause a bunch of conflicts

@vdemeester
Copy link
Collaborator Author

Discussed it with @dnephin, preparing a PR for the move of moby/moby/opts. I'll update this one after 👼

aaronlehmann
aaronlehmann reviewed May 13, 2017
View reviewed changes
opts/duration.go Outdated
)

// PositiveDurationOpt is an option type for time.Duration that uses a pointer.
// It bahave similarly to DurationOpt but only allows positive duration values.
Copy link
Contributor

Choose a reason for hiding this comment

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

"behave"

@aaronlehmann
Copy link
Contributor

Seems okay as a temporary step, but I think it's better to remove DurationOpt and have PositiveDurationOpt wrap a time.Duration. I believe the original purpose of DurationOpt was to to distinguish a default value from one specified by the user, but now we use flags.Changed for that. We mix time.Duration with DurationOpt and it's a bit inconsistent.

However, if you do this, make sure it doesn't affect the CLI help output, because I remember there were some differences between how they format defaults. IIRC, flags.DurationVar will include the default in the help output if the default value is nonzero. We generally don't want this for service update, where it suggests that by default certain values on the service will be changed. I don't see any defaults passed to flags.DurationVar anymore, so this may not be an issue, but it's something to watch out for.

@vdemeester vdemeester force-pushed the move-duration-opts branch 3 times, most recently from 9389df1 to 7bd5712 Compare May 16, 2017 15:24
@vdemeester
Copy link
Collaborator Author

Updated and rebase 👼

dnephin
dnephin reviewed May 16, 2017
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

build is failing

cli/command/service/opts.go Outdated
limitMemBytes opts.MemBytes
resCPU opts.NanoCPUs
resMemBytes opts.MemBytes
limitCPU dopts.NanoCPUs
Copy link
Contributor

Choose a reason for hiding this comment

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

dopts doesn't exist

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, bad rebase 😝

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be fixed and green 📗

Copy link
Member

Choose a reason for hiding this comment

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

lol, I was actually looking at that and scratching my head if I overlooked something 😄

@vdemeester
Move duration opts into an opts package
b5182ba 
They have nothing to do with service and could be used on their own.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
dnephin
dnephin approved these changes May 16, 2017
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@mavenugo mavenugo added this to the 17.06.0 milestone May 16, 2017
thaJeztah
thaJeztah approved these changes May 17, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

holding of merging, because there's some other PR's that are needed for 17.06, not sure if they're affected

@thaJeztah thaJeztah merged commit eea4a38 into docker:master May 18, 2017
@thaJeztah
Copy link
Member

removing the milestone, because I don't know if this ends up in 17.06 or 17.07

@thaJeztah thaJeztah removed this from the 17.06.0 milestone May 18, 2017
@vdemeester vdemeester deleted the move-duration-opts branch May 18, 2017 09:17
@thaJeztah
Copy link
Member

Looks to be 17.07; https://github.com/docker/docker-ce/blob/17.06/components/cli/cli/command/service/opts.go#L28-L45

@thaJeztah thaJeztah added this to the 17.07.0 milestone Jul 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronlehmann aaronlehmann aaronlehmann left review comments

@dnephin dnephin dnephin approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
status/2-code-review
Projects
None yet
Milestone
17.07.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vdemeester @mavenugo @dnephin @aaronlehmann @thaJeztah @GordonTheTurtle