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

Unify file handling for local and remote transports (#189) #190

Closed
wants to merge 1 commit into from

Conversation

tpcwang
Copy link

@tpcwang tpcwang commented Aug 23, 2017

Local and remote transports expose different file interfaces. This
is because local transport uses LinuxFile with some methods
overriden with Ruby's File calls whereas remote transport uses
the appropriate file classes from Train::Extras. This causes
problems when users are running locally on Windows because some
LinuxFile implementations are not appropriate there.

There are two potential compat breaks with this change:

  1. pw_username is no longer provided for local transport
  2. pw_groupname is no longer provided for local transport

These are not available for remote transport, so this doesn't
seem like it should be a problem for clients.

Fixes #189

Local and remote transports expose different file interfaces. This
is because local transport uses LinuxFile with some methods
overriden with Ruby's File calls whereas remote transport uses
the appropriate file classes from Train::Extras. This causes
problems when users are running locally on Windows because some
LinuxFile implementations are not appropriate there.

There are two potential compat breaks with this change:
1. pw_username is no longer provided for local transport
2. pw_groupname is no longer provided for local transport

These are not available for remote transport, so this doesn't
seem like it should be a problem for clients.

Signed-off-by: Ted Wang <ted.wang@ni.com>
@chris-rock
Copy link
Contributor

Thank you @tpcwang for providing this PR. It highlights an issue that we have with train on Windows.pw_username and pw_groupname are private functions and are not exposed.

I think this PR is going in a great direction. Nevertheless, I believe we should not give up the local implementations too early. It improves the speed if InSpec since we can use the Ruby file implementation instead of using commands to emulate the behavior.

Instead, I propose that we have a local_linux_file.rb and a local_windows_file.rb that overrides the methods that can be optimized.

@tpcwang
Copy link
Author

tpcwang commented Sep 5, 2017

Okay. I didn't realize local_file was added for performance reasons. I'll look at this again in the near future. @Vasu1105 if you get to this before I do in #193, I would recommend that you pull in the tests that I have created so that we have better coverage on Windows.

@Vasu1105
Copy link
Contributor

Vasu1105 commented Sep 6, 2017

ok @tpcwang

@clintoncwolfe
Copy link
Contributor

As this has been stale for a long time, and train has grown a great deal in that time, I'm going to go ahead and close this. Please d re-open if you feel that is best and you have time to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants