ggiamarchi / vagrant-openstack-provider

Use Vagrant to manage OpenStack Cloud instances.
MIT License
247 stars 101 forks source link

Switch to standard WaitForCommunicator middleware #281

Closed Sharpie closed 8 years ago

Sharpie commented 8 years ago

The OpenStack provider's action_up used a custom WaitForServerToBeAccessible action to poll instances via SSH in order to determine when the VM is up and running. One large shortcoming of this custom implementation is that it does not support communicators other than SSH, notably WinRM. This patch replaces the WaitForServerToBeAccessible with the built-in WaitForCommunicator action which has been part of Vagrant since v1.3 and is able to handle multiple communicators.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.2%) to 92.104% when pulling 4c8e4652891da87aa4548c3d72e80478c5643ed1 on Sharpie:use-waitforcommunicator into 88087367db434df92bedc2530745807c85e637c1 on ggiamarchi:master.

ggiamarchi commented 8 years ago

Nice, it's always a good idea to switch to a standard middleware.

You marked the legacy option ssh_timeout as deprecated (which is a good point) but the way you have implemented it, the parameter's value is just ignored. Actually, deprecated means the parameter is still working but will be remove in a future release.

Ignoring the option can be annoying for users because they will have unexpected behaviour after a plugin upgrade. It silently breaks the backward compatibility. As long as the option exists, its value must be taken into account.

What i would do here :

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.4%) to 91.837% when pulling 09fea1a9f750de952bfaa1b7f3b693babeb4b4e2 on Sharpie:use-waitforcommunicator into 7a5d972f05834a86849b36bd0fc8dd8123eb378b on ggiamarchi:master.

Sharpie commented 8 years ago

I re-factored the deprecation patch to copy ssh_timeout over to config.vm.boot_timeout if set and log a deprecation warning. Throwing an error if both are set by the user is difficult as config.vm.boot_timeout has a default value of 300.

ggiamarchi commented 8 years ago

LGTM