[MSYS-649] Fix InSpec file size in Windows, refactor File classes #193

Vasu1105 · 2017-08-28T11:00:45Z

Issue Ref: https://getchef.zendesk.com/agent/#/tickets/15734

Signed-off-by: Vasu1105 vasundhara.jagdale@msystechnologies.com

adamleff · 2017-08-28T11:18:01Z

Please only open a PR when you believe your code is complete and you are ready for a review. Thank you!

Vasu1105 · 2017-08-28T11:31:00Z

@adamleff that one line of change has fixed the issue which is mentioned on card but I need review on this since I am not very much familiar with code flow and specs are failing. Also I want to know how should I run all specs.

adamleff · 2017-08-28T11:34:23Z

OK - i'll re-open and edit your description. it stated it was still "in progress" and we do not keep PRs open that are not ready for review or in the process of being reviewed. Thank you :)

adamleff · 2017-08-28T11:36:04Z

@Vasu1105 to run the tests, you can do the following in the project top-level directory:

bundle install
bundle exec rake

That will run the unit tests and the lint tests, both of which are what are run via Travis. Thanks!

Vasu1105 · 2017-08-28T12:01:14Z

@adamleff I observed that using inspec file resource when I do check for file exist? for windows os it does not call file_window.rb in train for executing exist. It calls load_file executes https://github.com/chef/train/blob/master/lib/train/transports/local_file.rb#L17 line of code. So I am using the same approach but not sure why test cases are failing. Please add your inputs on this.

I am using following profile for testing

title 'filetest'
control 'filetest' do
impact 0.7
title 'file test'
describe file("C:\Users\azure\glass.pem") do
it { should exist }
its('size') { should eq 10741 }
end
end

chris-rock · 2017-08-28T12:16:57Z

Thank you @Vasu1105 for highlighting this issue. Unfortunately, I think the issue is a little bit more complex:

we should not return nil in https://github.com/chef/train/blob/master/lib/train/extras/file_windows.rb#L70-L76, instead a not implemented error
LocalFile cannot load Train::Extras::LinuxFile https://github.com/chef/train/blob/master/lib/train/transports/local_file.rb#L9 by default, it needs to decide if we are on windows or linux and load the appropriate file implementation, that needs to be fixed here: https://github.com/chef/train/blob/master/lib/train/transports/local.rb#L43

chris-rock · 2017-08-28T12:40:22Z

We probably want to combine this with #190

Vasu1105 · 2017-09-01T06:20:15Z

@chris-rock After conversation with @btm I am taking up the work for fixing larger issue as you mentioned since there is no movement on #190

we should not return nil in https://github.com/chef/train/blob/master/lib/train/extras/file_windows.rb#L70-L76, instead a not implemented error
LocalFile cannot load Train::Extras::LinuxFile https://github.com/chef/train/blob/master/lib/train/transports/local_file.rb#L9 by default, it needs to decide if we are on windows or linux and load the appropriate file implementation, that needs to be fixed here: https://github.com/chef/train/blob/master/lib/train/transports/local.rb#L43

May be I will need some inputs from you to understand code flow of train. Also what is the use case of local_file.rb

adamleff

I think we need to rethink the file classes a bit as a result of this change. @chris-rock is correct in that LocalFile cannot depend on LinuxFile, but we shouldn't overload FileCommon as a result.

This is just my opinion but I think we could be much clearer in the names of our classes (and even the file layout) to make this easier. For example, calling things "Extras" makes it seem like it's not important, but the code in there is really critical, and the hierachy isn't clear.

So here's how I'd resolve this in terms of class layout:

Train::File
├── Local
│     ├── Unix
│     └── Windows
└── Remote
      ├── Unix
      │     ├── AIX
      │     └── Linux
      └── Windows

A quick explanation:

Train::File is the current FileCommon class; it defines all the methods every other implementation of a Train::File must implement, including populating them with a raise NotImplementedError as needed. The class as-is is probably just fine.
Train::File::Local would define all the methods from Train::File with the appropriate calls from the Ruby stdlib. I think much of the content of Train::Transports::Local::Connection::File should actually move in here.
Train::File::Local::Unix and Windows can be used to override any of those methods as needed. These are likely not even necessary and I would not initially implement them.
Train::File::Remote::Unix and Windows would define all the Train::File methods but using the appropriate remote calls. They can further be subclasses like we do today (Linux off of Unix because of the different method for getting file content). The current contents of the Trains::Extras::UnixFile and friends would move in here.

Then essentially, the transport would instantiate the right class depending on the transport and the OS:

Train::File::Local
Train::File::Remote::AIX
Train::File::Remote::Unix
Train::File::Remote::Linux
Train::File::Remote::Windows

... and Train::Transports::Local::Connection::File would go away so that all file-related logic is contained within the Train::File hierarchy, no matter the transport.

@chris-rock, would love your input on this. Yes, this is VERY similar to what we have now, but it consolidates things and makes the intentions clearer.

adamleff · 2017-09-11T11:50:18Z

lib/train/extras/file_common.rb

@@ -20,6 +20,14 @@ class FileCommon # rubocop:disable Metrics/ClassLength
      end
    end

+    COMMON_FILE_METHODS = %w{exist? socket? directory? symlink? pipe?}.freeze
+
+    COMMON_FILE_METHODS.each do |m|


I don't believe these belong here. FileCommon should provide the interface that the other classes should implement. These methods are very specific to local files.

chris-rock · 2017-09-12T11:40:30Z

The structure @adamleff proposes makes a lot of sense to me! What we need to watch is the overlap between local and remote. We should try to keep the overlap as high as possible.

Vasu1105 · 2017-09-13T05:15:42Z

@adamleff and @chris-rock I am starting work on structural changes and will update the same PR after I am done with the changes. Thank you for the inputs.

adamleff · 2017-09-13T11:36:15Z

@Vasu1105 thank you for taking on this challenge! It will help the stability and clarity of this part of the Train codebase.

Please reach out if you have any questions or need any assistance.

Vasu1105 · 2017-09-13T12:30:08Z

so @adamleff the implementation under extras for file will go off right ? And all this will be classes right ?

adamleff · 2017-09-13T12:40:18Z

Yes, the file-related code under lib/train/extras would be removed and copied/adapted to the new class structure, likely under lib/train/file/

Vasu1105 · 2017-10-06T07:10:53Z

@adamleff & @chris-rock this PR is ready for review

adamleff

@Vasu1105 this is such a great improvement! Upon a quick review, I found a couple of clean-up things. But overall, this is precisely how I hoped it would look. Thank you!

adamleff · 2017-10-09T14:01:26Z

lib/train/file.rb

    end

-    def linked_to?(dst)
-      link_path == dst
+    # TODO: This methods needs to raise error if not implemented currently it set to nil since conversion


I don't think this is a "TODO" - this is a design choice. Since only Windows will populate this field and it's nil by default, this is a totally acceptable implementation. I think the comment should read something like:

# product_version is primarily used by Windows operating systems only and will be overwritten # in Windows-related classes. Since this field is returned for all file objects, the acceptable # default value is nil

adamleff · 2017-10-09T14:01:44Z

lib/train/file.rb

    end

-    def link_path
-      symlink? ? path : nil
+    # TODO: This methods needs to raise error if not implemented currently it set to nil since conversion


Same as above comment re: product_version

adamleff · 2017-10-09T14:04:33Z

lib/train/file/local/unix.rb

+        end
+
+        def stat
+          return @stat if defined? @stat


Slight nitpick, but we should be consistent with wrapping method arguments with parentheses...

return @stat if defined?(@stat)

adamleff · 2017-10-09T14:06:29Z

lib/train/file/local/windows.rb

+        end
+
+        def stat
+          return @stat if defined? @stat


Same comment re: wrapping arguments with parentheses.

adamleff · 2017-10-09T14:11:50Z

test/unit/transports/local_file_test.rb

-      end
-    end
-  end
+  # it 'gets file contents' do


Can we remove these commented-out tests?

Vasu1105 · 2017-10-10T06:28:02Z

@adamleff Thank you :). Fixed review comments

adamleff

@Vasu1105 this looks really good. Before I provide my approval, can you please rebase your changes against the latest master? It appears you have some merge conflict. Thank you!

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Vasu1105 · 2017-10-17T08:10:18Z

@adamleff I have resolved the conflicts. I need to refactor #203 as per new structure to resolve conflicts and it was not a simple conflict.

… changes as per new structure Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

adamleff

Looks great, @Vasu1105 - thanks!

chris-rock

This is great work and makes the code more structured. Thank you @Vasu1105

arlimus

Amazing improvements and change overall, really great work!!
I need a tiny iteration to see if we can't get out of indentation hell, but may not block it. Found a few tiny things, i'll look again tomorrow :)

arlimus · 2017-10-18T23:58:02Z

test/integration/tests/path_block_device_test.rb

-    it 'has no file_version' do
-      file.file_version.must_equal(nil)
+    it 'has no product_version' do
+      file.file_version.must_be_nil


the description talks about product_version vs the test looking at file version 😉

arlimus · 2017-10-18T23:58:17Z

test/integration/tests/path_character_device_test.rb

    end

-    it 'has no file_version' do
-      file.file_version.must_equal(nil)
+    it 'has no product_version' do


arlimus · 2017-10-18T23:58:30Z

test/integration/tests/path_file_test.rb

    end

-    it 'has no file_version' do
-      file.file_version.must_equal(nil)
+    it 'has no product_version' do


arlimus · 2017-10-18T23:58:47Z

test/integration/tests/path_folder_test.rb

    end

-    it 'has no file_version' do
-      file.file_version.must_equal(nil)
+    it 'has no product_version' do


Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Vasu1105 · 2017-10-23T05:28:59Z

Thank you @chris-rock @arlimus :) . @arlimus I have fixed the review comments.

arlimus · 2017-10-24T17:51:08Z

Sweet thank you so much @Vasu1105 i'll take another look today on one more point i was interested in. Almost at the finish line :D

arlimus

Thank you for the adjustments!!

Vasu1105 · 2017-10-30T05:53:19Z

Thank you @adamleff , @stevendanna and @chris-rock, @arlimus and all others for help for this work :)

chris-rock · 2017-10-30T09:28:37Z

Great work @Vasu1105 !

During the file refactor in #193, an error was made in class names that caused AIX file operations to be severely limited and QNX file support was broken. This restores the AIX and QNX remote file functionality and adds unit tests for each of those classes. Signed-off-by: Adam Leff <adam@leff.co>

adamleff closed this Aug 28, 2017

adamleff reopened this Aug 28, 2017

adamleff added the in progress label Aug 28, 2017

tpcwang mentioned this pull request Sep 5, 2017

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

Closed

btm requested review from chris-rock and adamleff September 7, 2017 15:13

btm removed the in progress label Sep 7, 2017

adamleff suggested changes Sep 11, 2017

View reviewed changes

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch from 49b4e6a to 627c79e Compare September 19, 2017 11:03

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch 3 times, most recently from 7012db5 to 86a217e Compare September 29, 2017 09:41

stevendanna mentioned this pull request Oct 3, 2017

File type comparisons only accept symbols despite documentation inspec/inspec#2204

Closed

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch from 5cb3af3 to 5d359fe Compare October 5, 2017 10:23

adamleff suggested changes Oct 9, 2017

View reviewed changes

adamleff suggested changes Oct 16, 2017

View reviewed changes

Vasu1105 added 3 commits October 17, 2017 11:09

Moved methods to respective classes for file and fixed specs

cedcf50

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Removed local_file.rb

6b8c2ef

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Fixed review comments

357b401

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch from 4071a54 to c50add7 Compare October 17, 2017 06:10

Vasu1105 requested a review from a team October 17, 2017 06:10

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch from 7ba5a32 to a622ce0 Compare October 17, 2017 06:46

Resolved conflicts after rebasing master and updated qnx file support…

b465802

… changes as per new structure Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Vasu1105 force-pushed the vasundhara/fix_for_inspec_file_size_for_windows_os branch from a622ce0 to b465802 Compare October 17, 2017 08:22

adamleff changed the title ~~[MSYS-649] Fix for Inspec file size test in windows fails~~ [MSYS-649] Fix InSpec file size in Windows, refactor File classes Oct 17, 2017

adamleff approved these changes Oct 17, 2017

View reviewed changes

chris-rock requested a review from arlimus October 17, 2017 13:03

chris-rock approved these changes Oct 17, 2017

View reviewed changes

arlimus suggested changes Oct 18, 2017

View reviewed changes

Fixed review comments

7c0ac00

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

arlimus approved these changes Oct 30, 2017

View reviewed changes

arlimus merged commit 57387de into inspec:master Oct 30, 2017

adamleff mentioned this pull request Feb 2, 2018

Fix AIX and QNX file support #249

Merged

[MSYS-649] Fix InSpec file size in Windows, refactor File classes #193

[MSYS-649] Fix InSpec file size in Windows, refactor File classes #193

Conversation

Vasu1105 commented Aug 28, 2017 • edited by adamleff Loading

adamleff commented Aug 28, 2017

Vasu1105 commented Aug 28, 2017 • edited Loading

adamleff commented Aug 28, 2017

adamleff commented Aug 28, 2017

Vasu1105 commented Aug 28, 2017 • edited Loading

chris-rock commented Aug 28, 2017 • edited Loading

chris-rock commented Aug 28, 2017

Vasu1105 commented Sep 1, 2017 • edited Loading

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock commented Sep 12, 2017

Vasu1105 commented Sep 13, 2017

adamleff commented Sep 13, 2017

Vasu1105 commented Sep 13, 2017 • edited Loading

adamleff commented Sep 13, 2017

Vasu1105 commented Oct 6, 2017 • edited Loading

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vasu1105 commented Oct 10, 2017

adamleff left a comment

Choose a reason for hiding this comment

Vasu1105 commented Oct 17, 2017

adamleff left a comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vasu1105 commented Oct 23, 2017 • edited Loading

arlimus commented Oct 24, 2017

arlimus left a comment

Choose a reason for hiding this comment

Vasu1105 commented Oct 30, 2017 • edited Loading

chris-rock commented Oct 30, 2017

Vasu1105 commented Aug 28, 2017 •

edited by adamleff

Loading

Vasu1105 commented Aug 28, 2017 •

edited

Loading

Vasu1105 commented Aug 28, 2017 •

edited

Loading

chris-rock commented Aug 28, 2017 •

edited

Loading

Vasu1105 commented Sep 1, 2017 •

edited

Loading

Vasu1105 commented Sep 13, 2017 •

edited

Loading

Vasu1105 commented Oct 6, 2017 •

edited

Loading

Vasu1105 commented Oct 23, 2017 •

edited

Loading

Vasu1105 commented Oct 30, 2017 •

edited

Loading