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

Small cleanup now that we have multiple details about the server stored on the cli #31830

Merged
merged 1 commit into from
Mar 29, 2017
Merged

Small cleanup now that we have multiple details about the server stored on the cli #31830

merged 1 commit into from
Mar 29, 2017

Conversation

dnephin
Copy link
Member

@dnephin dnephin commented Mar 14, 2017

No description provided.

vdemeester
vdemeester approved these changes Mar 15, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@ehazlett
Copy link
Contributor

@dnephin rebase please, thx!

@dnephin
Copy link
Member Author

dnephin commented Mar 17, 2017

rebased

@ehazlett
Copy link
Contributor

LGTM (nam)

@cpuguy83
Copy link
Member

ServerType type (and subsequent interface method) is a bit weird.
Other than that it looks ok.

@thaJeztah
Copy link
Member

Thinking.. are ServerDetails, ServerProperties, ServerInfo better options?

@cpuguy83
Copy link
Member

ServerInfo seems better.

@dnephin
Create a new ServerType struct for storing details about the server o…
4152243 
…n the client.

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

dnephin commented Mar 20, 2017

SGTM, I changed the name to ServerInfo

@coolljt0725
Copy link
Contributor

LGTM ping @cpuguy83 @thaJeztah

thaJeztah
thaJeztah approved these changes Mar 29, 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

@thaJeztah thaJeztah merged commit bce494f into moby:master Mar 29, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Mar 29, 2017
@dnephin dnephin deleted the refactor-cli-state branch March 29, 2017 16:11
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@thaJeztah
Merge pull request moby#31830 from dnephin/refactor-cli-state
9cfbcd4 
Small cleanup now that we have multiple details about the server stored on the cli
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@thaJeztah
Merge pull request moby#31830 from dnephin/refactor-cli-state
db8f631 
Small cleanup now that we have multiple details about the server stored on the cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@coolljt0725 coolljt0725

Labels
status/2-code-review
Projects
None yet
Milestone
17.05.0
Development

Successfully merging this pull request may close these issues.
7 participants
@dnephin @ehazlett @cpuguy83 @thaJeztah @coolljt0725 @vdemeester @GordonTheTurtle