inspec / kitchen-inspec

Test-Kitchen Plugin for InSpec
Other
109 stars 58 forks source link

0.22.0 breaks transport-rsync #160

Closed stefanandres closed 6 years ago

stefanandres commented 6 years ago

https://github.com/unibet/kitchen-transport-rsync doesn't work anylonger with 0.22.0

06:33:53  | >>>>>>     Failed to complete #verify action: [Verifier Inspec does not support the Rsync Transport] on blackhole-ubuntu-1404

Is there any substitution for this? We switched to rsync because the default method of transfering files way too slow.

clems4ever commented 6 years ago

It also broke the speedyssh transport available here: https://github.com/criteo/kitchen-transport-speedy.

The problem is due to https://github.com/chef/kitchen-inspec/commit/20cb81c2bd4d476c3519f9ec503a7cd50efb3e45. According to the commit message, monkeypatching is the new way of implementing new transports. Wouldn't it be better to test if a Kitchen::Transport::Custom value is defined instead of forcing implementations to monkeypatch?

lehn-etracker commented 6 years ago

It also breaks sftp transport: https://github.com/coderanger/kitchen-sync 😿

gordonbondon commented 6 years ago

I've submitted a fix for speedy ssh criteo/kitchen-transport-speedy#3

chris-rock commented 6 years ago

Monkey Patching is not the preferred route. Lets find a better way. I'd like to understand your needs and the way you would like to integrate.

davestj commented 6 years ago

confirmed for sshtar as well Failed to complete #verify action: [Verifier Inspec does not support the Sshtar Transport]

ricoli commented 6 years ago

At least in my case I use rsync transport because SSH transport is quite flaky on EC2 (https://github.com/test-kitchen/kitchen-ec2/issues/288), so it would be great if my provisioner could still use rsync, but my verifier could use SSH as transport. For now I went back to using v0.20.0...

ChadScott commented 6 years ago

Exactly as @ricoli said, using sftp/ssh for each file is very time consuming (and sometimes simply fails) when you have to sync over a large file tree. Rsync handles this naturally and quite quickly.

brodygov commented 6 years ago

@chris-rock Was there a resolution here?

rojomisin commented 6 years ago

having this issue too not easy to roll back when using chefdk

coderanger commented 6 years ago

@chris-rock Monkey-patching is absolutely the preferred route, that was the whole point :)

rhyas commented 6 years ago

This is apparently still broken in 0.23.0. Will this get fixed?

coderanger commented 6 years ago

@rhyas It has to be fixed in the transport plugin, barring someone having time to do another pass on kitchen-inspec, which is not planned at this time.

rhyas commented 6 years ago

I'm not sure why that would be the transport modules problem? Kitchen-inspec went from testing by a class, to testing by a name, and it appears to have broken a transport module that inherited from a class to make things simple. (and thus passed the class test) There's nothing really to change in the RSync module that I can see, it's all on this module to add the helper functions to duplicate Rsync with Ssh to allow the rsync module by name and support Rsync. Am I missing something completely?

coderanger commented 6 years ago

@rhyas The flip side is that testing by class broke anything that didn't inherit from one of the included transports.

rhyas commented 6 years ago

Well yes, but that was a good thing. It meant this module only supported, and tried to use, transports that were based on modules it understood and supported. (: Regardless of the semantics, it's still a fix in this module to make rsync work though. At the moment, it only supports Ssh, Winrm, and Dokken because those are the only modules hard coded with the options by name. (runner_optionsfor#{transport.name.downcase})

coderanger commented 6 years ago

It supports my transports just fine, as you can see in https://github.com/coderanger/kitchen-sync/blob/master/lib/kitchen/transport/sftp.rb#L37-L45 and similar. Add something similar to whatever transport you want to use. The idea is to not guess how non-standard transports work, you need to handle it explicitly.

rhyas commented 6 years ago

👍 Gotcha. Probably a more elegant way to support transports. From a user perspective, this seems weak, changing stuff and expecting other projects that otherwise work fine to make code changes to conform to your way of doing things. Guess that's why people will just keep using 0.20.0. Someone should probably close this as "Won't Fix". (:

stefanandres commented 6 years ago

kitchen-sync indeed works, but is still slower than kitchen-transport-rsync, so I've tried kitchen-transport-speedy and that seems to fulfil my requirements for now. :-)