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

Rename "none" driver to "test" #2437

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@jeanlaurent
Member

jeanlaurent commented Nov 27, 2015

Following @ehazlett advice here this PR rename the none driver to test, and remove it from the release package, set the generic driver as default driver and remove the documentation related to it. We keep using it to run the various integration test, where it is of great help in allowing a faster execution time.

The driver none in it's current state is not usable. The hardcoded ssh user is a blank string the ssh port is 0, and those can not be overriden.

The main purpose of this driver,or at least how it was partially documented to, is to register an existing running docker instance, which is what the generic driver do.

This will close #2270, #2355, #1532, #1435 and #643

@dgageot

This comment has been minimized.

Collaborator

dgageot commented Nov 27, 2015

I would add that everything that targets an existing host is done with the generic driver. If this driver is not good enough (I'm thinking documentation, supporting non TLS, not restarting the docker daemon...), then we should create issues for the generic driver.

Rename "none" driver to "test"
Signed-off-by: Jean-Laurent de Morlhon <jeanlaurent@morlhon.net>
@jeanlaurent

This comment has been minimized.

Member

jeanlaurent commented Nov 28, 2015

This is ready for review.

ping @docker/machine-maintainers

@dgageot

This comment has been minimized.

Collaborator

dgageot commented Nov 28, 2015

LGTM

@jeanlaurent jeanlaurent added this to the 0.5.2 milestone Nov 30, 2015

@jeanlaurent

This comment has been minimized.

Member

jeanlaurent commented Nov 30, 2015

We don't want to push this without you taking a glance @nathanleclaire . But this would help for the release since it ends the 'none' driver misunderstanding.

@dnephin

This comment has been minimized.

dnephin commented Nov 30, 2015

Isn't the none driver the only way to support a daemon without SSH ? This is really useful for anyone that has local socket, or has already provisioned some instances and just wants a way to add them to machine.

It would be good to add this support to some other driver before removing what little support exists.

cc @mikedougherty

@nathanleclaire nathanleclaire modified the milestones: 0.5.3, 0.5.2 Nov 30, 2015

@nathanleclaire

This comment has been minimized.

Contributor

nathanleclaire commented Nov 30, 2015

Yes, none is and always has been targeted at exactly the use case @dnephin mentions: Adding an existing instance of the Docker daemon to be managed in Machine's store with necessarily needing SSH access, provider credentials, etc. The none driver being used for testing was incidental.

@jeanlaurent Can we please open a thread and discuss what's to be done about the none driver before making any sudden moves like this?

My feeling is that the communication with the community and existing users of the none driver has not been thorough. I'd prefer to chat more with folks and get everyone on a similar page before dropping support for the feature completely, and/or redirecting it to a different expected workflow.

@nathanleclaire

This comment has been minimized.

Contributor

nathanleclaire commented Nov 30, 2015

Note that Docker's existing feature deprecation policy indicates that such an action (deprecating none in favor of generic) is usually expected to take a timespan of 2 releases.

@jeanlaurent jeanlaurent self-assigned this Dec 1, 2015

@dgageot

This comment has been minimized.

Collaborator

dgageot commented Dec 1, 2015

@nathanleclaire I'm wondering when none driver has ever worked. Do you have an idea? I understand that people complain if we remove something that works. But something that never really worked. cc @ehazlett

@mikedougherty

This comment has been minimized.

mikedougherty commented Dec 1, 2015

@dgageot My understanding is that it worked prior to 0.5.0. #2270 has some more context.

Even if it never worked, I would personally rather see the feature fixed than removed, per @dnephin's comment:

This is really useful for anyone that has local socket, or has already provisioned some instances and just wants a way to add them to machine.

@metasim

This comment has been minimized.

metasim commented Dec 8, 2015

@dgageot none worked in 0.4.1. Have had to roll back to regain access to my infrastructure. :-(

@jeanlaurent jeanlaurent removed this from the 0.5.3 milestone Dec 10, 2015

@jeanlaurent

This comment has been minimized.

Member

jeanlaurent commented Dec 23, 2015

Closing will, re-open if needed by #2667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment