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

fixes #1441 set default schema to tcp for docker host #1443

Merged
merged 2 commits into from
Oct 17, 2018

Conversation

lifubang
Copy link
Contributor

@lifubang lifubang commented Oct 12, 2018

Signed-off-by: Lifubang [email protected]
- What I did
Please see #1441
There is a traditional command "docker -H :2375 ps" in docker cli, but in master, it can't work now.
Is this feature removed in plan or just an error?

- How I did it
I think this may be a bug after merge #1014
add case ssh to dopts.ParseHost, together with all other schema.

- How to verify it
see #1441

fixes #1441
fixes moby/moby#38118

- Description for the changelog
set default schema to tcp for docker host param when there is no schema

- A picture of a cute animal (not mandatory but encouraged)
p08ehdk zh d s d426fzw

@lifubang lifubang changed the title set default schema to tcp for docker host fixes #1441 set default schema to tcp for docker host Oct 12, 2018
@codecov-io
Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #1443 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1443      +/-   ##
==========================================
- Coverage    54.2%   54.19%   -0.01%     
==========================================
  Files         289      289              
  Lines       19378    19377       -1     
==========================================
- Hits        10503    10502       -1     
  Misses       8199     8199              
  Partials      676      676

@lifubang
Copy link
Contributor Author

I have update a more reasonable schema parse method.
ping @AkihiroSuda @cpuguy83 PTAL

@lifubang
Copy link
Contributor Author

Code looks good to me, thank you! But I wonder if we can add a regression test here on this feature.

@silvin-lubecki Thank you for your review.
It's a good idea to add a test case which can avoid such error.

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

Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit a2a7a7c into docker:master Oct 17, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Oct 17, 2018
@lifubang
Copy link
Contributor Author

Shall we need to merge to 18.09? Otherwise the version 18.09 will become the only version which doesn't support default host schema.

@thaJeztah
Copy link
Member

@lifubang yes, I had this one on my list to backport to 18.09

@cpuguy83
Copy link
Collaborator

Can you open a pr to the release branch?

@thaJeztah
Copy link
Member

@cpuguy83 almost done 😅

@cpuguy83
Copy link
Collaborator

haha, posting at the same time.

@thaJeztah
Copy link
Member

backport is up #1454

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.

Documentation needed on DOCKER_HOST behavior change docker -H :2375 can't work now
8 participants