Platform framework and detect DSL #204

jquick · 2017-10-26T16:37:40Z

This change moves the detect logic for platforms into a new framework. You can create platforms on the fly using the new platform DSL. There are still some detect tests to write for specific platforms.

For more info about this RFC please visit inspec/inspec#1661

Signed-off-by: Jared Quick <jquick@chef.io>

jquick · 2017-10-26T16:41:31Z

lib/train/platforms/detect/os_common.rb

+    include Train::Platforms::Detect::Linux
+    include Train::Platforms::Detect::Windows
+
+    def rbconfig(regex)


I don't like this name as this will be part of what you can use in the detect logic. Maybe "get_host_from_ruby" or something?

How about rbconfig_host_os or ruby_host_os? Not a fan of get_ methods personally.

i like ruby_host_os

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff

@jquick this is pretty stellar work for your first time mucking around in Train/InSpec. Nicely done.

I have a bunch of style feedback and questions, and I'm happy to get on a Zoom and talk through them if needed. Ultimately, the structure, concepts, and layout all make sense to me and I'm happy to see them in this state. Great work.

adamleff · 2017-10-31T16:36:47Z

lib/train/platform.rb

+module Train
+  class Platform
+    include Train::Platforms::Common
+    attr_accessor :condition, :families, :backend, :platform, :family_hierarchy


Nitpick warning, but could I implore you to sort these alphabetically so, as they grow/shrink, it's easier to find what you're looking for?

adamleff · 2017-10-31T16:38:35Z

lib/train/platform.rb

+      @family_hierarchy = []
+      @platform = {}
+      @detect = nil
+      @title = name =~ /^[[:alpha:]]+$/ ? name.capitalize : name


Curious as to why this match is needed. Even if the name is just a number, making it a string and capitalizing it won't throw an error.

Would it be simpler to make it this?: @title = name.to_s.capitalize

adamleff · 2017-10-31T16:58:13Z

lib/train/platform.rb

+      @platform
+    end
+
+    # Add genaric family? and platform methods to an existing platform


s/genaric/generic

(at least you know I read the comments, too 🙂 )

adamleff · 2017-10-31T16:59:38Z

lib/train/platform.rb

+      family_list = Train::Platforms.families
+      family_list.each_value do |k|
+        next if respond_to?(k.name + '?')
+        define_singleton_method(k.name + '?') {


Can we use do ... end for these blocks that consume multiple lines?

adamleff · 2017-10-31T16:59:43Z

lib/train/platform.rb

+      # Helper methods for direct platform info
+      @platform.each_key do |m|
+        next if respond_to?(m)
+        define_singleton_method(m) {


Can we use do ... end for these blocks that consume multiple lines?

adamleff · 2017-10-31T17:20:46Z

lib/train/platforms/detect/scanner.rb

+    def scan_children(parent)
+      parent.children.each do |plat, condition|
+        next if plat.detect.nil?
+        result = instance_eval(&plat.detect)


Same as above re: unused variable result with the exception of seeing if it's true or not.

adamleff · 2017-10-31T17:22:39Z

lib/train/platforms/detect/scanner.rb

+    end
+
+    def scan_family_children(plat)
+      child_result = scan_children(plat) if !plat.children.nil?


We should be consistent with our uses of if and unless... since you do a bunch of unless blah.nil?, we should do that here too:

child_result = scan_children(plat) unless plat.children.nil?

adamleff · 2017-10-31T17:23:54Z

lib/train/platforms/family.rb

+      @families = {}
+      @children = {}
+      @detect = nil
+      @title = name =~ /^[[:alpha:]]+$/ ? "#{name.capitalize} Family" : name


Same question as above re: Platform name/title -- I'm curious as to why this regex match is necessary.

adamleff · 2017-10-31T17:27:31Z

lib/train/platforms/specifications/os.rb

@@ -0,0 +1,456 @@
+# encoding: utf-8


Stylistically, this file reads really weird to me because it basically takes action when it's required. We don't really do anything else like that without our codebase.

I'm wondering if it would make sense to wrap this in a class/method and call that in lib/train/platforms.rb

For example:

module Train::Platforms class Specifications def self.load_specifications plat = Train::Platforms plat.family('windows') .detect # .... etc .... end end end

... and then at the very bottom of lib/train/platforms:

Train::Platforms::Specifications.load_specifications

Honestly, this file just looks like a left-behind test file even though it's the meat of our data mapping 🙂

What do you think?

I moved this to a proper class. I decided to load it on the transport creation as we are going to have the API transports require a different set of specifications.

adamleff · 2017-10-31T17:29:12Z

lib/train/transports/local.rb

+      def local?
+        true
+      end
+
      def os


Since we're not adding any value by having this method defined (i.e. we're not adding a deprecation warning), can we just call alias instead and alias os to platform with a comment added as to why we're doing it (i.e. because InSpec still expects it to be os).

Same comment applies to all the other Transport classes.

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff · 2017-10-31T20:23:11Z

lib/train/platforms/detect/os_common.rb

    end

    def unix_file_exist?(path)
      @backend.run_command("test -f #{path}").exit_status.zero?
    end

+    def uname_call(cmd)


This is really just run_command now, rather than uname-specific... maybe we should change the method name and that way it can be used for other things too?

Signed-off-by: Jared Quick <jquick@chef.io>

chris-rock

Great work @jquick I added some questions.

chris-rock · 2017-11-01T11:48:48Z

lib/train/transports/docker.rb

    end

+    # we need to keep os as a method for backwards compatibility with inspec
+    alias os platform


Should we do this in our base connection? This ensures that it is consistent across all connections and the implementations can focus on using platform

maybe we need a os_transport then?

This would be fine, if we are not going to be using BaseConnection for API.

We can also move it to base for now and add a os connection, once we add api

chris-rock · 2017-11-01T11:50:01Z

lib/train/transports/docker.rb

@@ -56,6 +56,7 @@ def reuse_connection
 class Train::Transports::Docker
  class Connection < BaseConnection
    def initialize(conf)
+      Train::Platforms::Specifications::OS.load_specifications


Should we move this to base connection?

The reason it was done on a per transport level was that we will likely have a Train::Platforms::Specifications::API.load_specifications for http and api transports. I don't think we want to load the OS for all transports including API ones.

That is if we decide to sub the BaseConnection for API as well. Maybe we make a new one for that.

chris-rock · 2017-11-01T11:52:32Z

lib/train/transports/ssh_connection.rb

@@ -55,10 +56,13 @@ def close
      @session = nil
    end

-    def os
-      @os ||= OS.new(self)
+    def platform


Since the method calls are more similar now, we could move that to base connection too?

chris-rock · 2017-11-01T11:56:15Z

lib/train/platforms/detect/os_common.rb

@@ -0,0 +1,56 @@
+# encoding: utf-8
+
+require 'train/platforms/detect/os_linux'


It feels like os_common.rb, os_linux.rb and os_windows.rb are just detect helper, the actual detection is done with the specifications. Am I right?

This is correct, they are just helper methods that can be used inside the detection blocks.

Should we make this more clear in our naming?

chris-rock · 2017-11-01T11:59:25Z

lib/train/platforms/detect/scanner.rb

+      # start with the platform/families who have no families (the top levels)
+      top = Train::Platforms.top_platforms
+      top.each do |_name, plat|
+        next unless plat.detect


Should we implement a method detect in family.rb? This would abstract the issue for us here. Also I am not sure if we want to use lambda for detect, so that we do not need to call instance_eval

I have added the false lambda instead of the check for detect. This way everything can be evaluated accordingly.
As far as moving away from instance_eval/exec. The reason we are doing this is so the detect logic will have access to all the helper methods (os_common, os_linux, etc...) in the detect context. We may be able to require all this logic in each instance but that seems overkill for the memory and really doing the same thing under the hood. This also makes it easy to create custom platforms on the fly via inspec or other instances. After talking it over and due to its controlled nature we are voting to keep it for now. I can comment it up to explain a bit more in code.

chris-rock · 2017-11-01T12:26:53Z

lib/train/platform.rb

@@ -0,0 +1,80 @@
+# encoding: utf-8


Should this file not be located in lib/train/platforms/platform?

Yeah, I do think that should be moved in after thinking about it.

Signed-off-by: Jared Quick <jquick@chef.io>

Signed-off-by: Adam Leff <adam@leff.co>

jquick added 10 commits October 13, 2017 09:02

Initial building block for platforms

dd0b848

Refactor platform classes and deprecate os

b4aac00

Signed-off-by: Jared Quick <jquick@chef.io>

Add basic detect logic

62b9d11

Signed-off-by: Jared Quick <jquick@chef.io>

Move in linux detect logic

922a716

Signed-off-by: Jared Quick <jquick@chef.io>

Add in local detect and more unix checks

f7567a7

Signed-off-by: Jared Quick <jquick@chef.io>

Refactor os specifications and some cleaning

d296abc

Signed-off-by: Jared Quick <jquick@chef.io>

Added windows and finalize detect

c7ae1a2

Signed-off-by: Jared Quick <jquick@chef.io>

Add dynamic family and platform methods and refactor os specification

ab3a04c

Signed-off-by: Jared Quick <jquick@chef.io>

add unit tests and finalize detect logic

83d4aad

Signed-off-by: Jared Quick <jquick@chef.io>

fix lint issues

efdab1d

Signed-off-by: Jared Quick <jquick@chef.io>

jquick requested a review from a team October 26, 2017 16:37

adamhjk added the in progress label Oct 26, 2017

jquick requested a review from adamleff October 26, 2017 16:38

jquick commented Oct 26, 2017

View reviewed changes

jquick added 3 commits October 26, 2017 17:54

add unknown fallback incase a platform option is not detected

6c27336

Signed-off-by: Jared Quick <jquick@chef.io>

Merge branch 'master' into jq/platform_objects

aff0d80

Signed-off-by: Jared Quick <jquick@chef.io>

Merge branch 'master' into jq/platform_objects

83e01ab

chris-rock requested review from chris-rock and arlimus October 27, 2017 07:31

jquick added 2 commits October 30, 2017 10:07

Merge branch 'master' into jq/platform_objects

810b3d6

Signed-off-by: Jared Quick <jquick@chef.io>

add skip empty line on os_release detect

7fd444d

Signed-off-by: Jared Quick <jquick@chef.io>

jquick force-pushed the jq/platform_objects branch from 3179679 to 7fd444d Compare October 30, 2017 14:29

removed unneeded platform type

021e6fd

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff suggested changes Oct 31, 2017

View reviewed changes

fix style issues and clear up logic

1c50340

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff reviewed Oct 31, 2017

View reviewed changes

style fixes and wrapped os specifications in a class

7cc0267

Signed-off-by: Jared Quick <jquick@chef.io>

chris-rock reviewed Nov 1, 2017

View reviewed changes

move around platform files into more logical spots

453a51d

Signed-off-by: Jared Quick <jquick@chef.io>

jquick force-pushed the jq/platform_objects branch from 83e3599 to 453a51d Compare November 1, 2017 18:19

jquick added 4 commits November 1, 2017 14:34

move redhat check to be at the end of the family as a catchall

98c10dd

Signed-off-by: Jared Quick <jquick@chef.io>

update raspbian detect to also check in os_release

64a0ccd

Signed-off-by: Jared Quick <jquick@chef.io>

update suse detect to be case insensitive

42e6dff

Signed-off-by: Jared Quick <jquick@chef.io>

add helper comments and refactor transport test

0425afb

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff approved these changes Nov 8, 2017

View reviewed changes

update detect logic for solaris and arista

babc875

Signed-off-by: Jared Quick <jquick@chef.io>

jquick force-pushed the jq/platform_objects branch from d76d0f4 to babc875 Compare November 9, 2017 19:43

jquick merged commit babc875 into master Nov 13, 2017

adamleff added a commit that referenced this pull request Nov 13, 2017

Reverting accidental push to master re: #204

cd5e3b2

Signed-off-by: Adam Leff <adam@leff.co>

chris-rock pushed a commit that referenced this pull request Nov 13, 2017

Reverting accidental push to master re: #204 (#208)

2f6a91e

Signed-off-by: Adam Leff <adam@leff.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform framework and detect DSL #204

Platform framework and detect DSL #204

jquick commented Oct 26, 2017

jquick Oct 26, 2017

jerryaldrichiii Oct 26, 2017

jquick Oct 26, 2017

adamleff left a comment

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

adamleff Oct 31, 2017

jquick Oct 31, 2017 •

edited

Loading

adamleff Oct 31, 2017

adamleff Oct 31, 2017

chris-rock left a comment

chris-rock Nov 1, 2017

chris-rock Nov 1, 2017

jquick Nov 1, 2017

chris-rock Nov 1, 2017

chris-rock Nov 1, 2017

jquick Nov 1, 2017 •

edited

Loading

chris-rock Nov 1, 2017

chris-rock Nov 1, 2017

jquick Nov 1, 2017

chris-rock Nov 1, 2017

chris-rock Nov 1, 2017

jquick Nov 1, 2017

chris-rock Nov 1, 2017

jquick Nov 1, 2017

		@@ -0,0 +1,56 @@
		# encoding: utf-8

		require 'train/platforms/detect/os_linux'

Platform framework and detect DSL #204

Platform framework and detect DSL #204

Conversation

jquick commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

jquick Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock 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

jquick Nov 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick Oct 31, 2017 •

edited

Loading

jquick Nov 1, 2017 •

edited

Loading