filesystem resource: inspect linux filesystems #2441

tarcinil · 2018-01-05T01:43:23Z

adding df resource

describe df('/') do
  its('size') { should be >= 32000 }
end

I do convert the cmd.stdout to an integer. I believe that as a full-blown resource, that pattern was better rather than relying on cmp to deal with it.

pulling chef:master

Signed-off-by: Vern Burton <[email protected]>

adamleff

Great first pass, @tarcinil! Left you a bunch of comments, let me know if anything is unclear.

adamleff · 2018-01-05T20:33:55Z

lib/resources/df.rb

+
+    def initialize(partition)
+      @partition = partition
+      @cmd = inspec.command("df #{@partition} --output=size | sed \"/blocks/d; s/ *//g\"")


Since you made partition an attr_reader (and it's available as partition as a method argument), you can drop the @ here.

adamleff · 2018-01-05T20:36:13Z

lib/resources/df.rb

+
+    def initialize(partition)
+      @partition = partition
+      @cmd = inspec.command("df #{@partition} --output=size | sed \"/blocks/d; s/ *//g\"")


I don't think there's any value in storing the command in an instance variable since it's only ever used in the size method... other than to perhaps prevent the command from being executed twice.

Instead, we can memoize the size value in the size method to avoid this. Example below.

adamleff · 2018-01-05T20:37:41Z

lib/resources/df.rb

+
+    def initialize(partition)
+      @partition = partition
+      @cmd = inspec.command("df #{@partition} --output=size | sed \"/blocks/d; s/ *//g\"")


--output=size is not a globally available flag. For example, it doesn't work on macOS or FreeBSD, to my knowledge.

We need to use the new supports keyword here to limit this resource to only executing on the operation systems on which this is supported. I would recommend we start with the "linux" family to start. @jquick, could you chime in here and help @tarcinil with the proper supports call that will accomplish this?

I was unable to find an existing example of supports as it pertained to resources.

I used a pre-existing pattern of inspec.os.linux? which appears to depend on chef/train in the background.

I have found the supports?/supported? functions from lib/inspec/plugins/resource.rb and lib/resources/platform.rb, but I am unable to find documentation that would allow me to implement it. I await implementation details from @jquick.

Hey @tarcinil. You are correct that the documentation is lacking for the new supports features for resources, this will be addressed. This was implemented via #1661 (section 4). For this resource you will want to add something like:
supports os_family: 'linux'
After you set the 'name' on line 3 in your new resource. This will skip any tests using this resource unless the platform is a linux base.

adamleff · 2018-01-05T20:40:41Z

lib/resources/df.rb

+    end
+
+    def size
+      @cmd.stdout.to_i


So, rather than storing the command object in an instance var, I'd change this to something like this:

@size ||= begin cmd = inspec.command("df #{@partition} --output=size | sed \"/blocks/d; s/ *//g\"").stdout raise Inspec::Exceptions::ResourceFailed, "Unable to get available space for partition #{partition}" if cmd.stdout.nil? || cmd.stdout.empty? || !cmd.exit_status.zero? cmd.stdout.to_i end

This will ensure the command is only run once (as we'll cache the size in an instance variable) and also does some additional error handling.

I'd also suggest just getting the raw output from the command and doing the string manipulation/parsing in Ruby instead of piping to sed.

adamleff · 2018-01-05T20:42:30Z

lib/resources/df.rb

+    end
+
+    def to_s
+      "DiskSpace #{@partition}"


Same here - can just change this to #{partition} thanks to it being made into an attr_reader

adamleff · 2018-01-05T20:42:49Z

test/unit/resources/df_test.rb

@@ -0,0 +1,22 @@
+# encoding: utf-8
+# author: Christoph Hartmann


You can remove these two author lines.

Signed-off-by: Vern Burton <[email protected]>

* removing author lines * using attr_reader functions * using ruby string functions rather than pipe to sed * adding os family detection * using ResourceFailed as the pattern already existed for OS family detection * using if for future case support for unix and unix-like (FreeBSD) Signed-off-by: Vern Burton <[email protected]>

… resource says that it is not supported on windows/unix. Signed-off-by: Vern Burton <[email protected]>

tarcinil · 2018-01-11T23:38:19Z

@adamleff Comments have been addressed, ready for another review.

adamleff

This is looking really solid, @tarcinil! One last small fix and we'll be good to go, I think.

adamleff · 2018-01-13T00:48:37Z

lib/resources/df.rb

+      if inspec.os.linux?
+        command = "df #{partition} --output=size"
+      else
+        raise Inspec::Exceptions::ResourceFailed, 'The `df` resource is not supported on your OS.'


The supports stuff you added takes care of this for you so you can remove all of this logic and just declare your command. 🙂

And you should move the command into the @size memoifier so it looks something like this and is the first line of the size method:

@size ||= begin cmd = inspec.command("df #{partition} --output=size") raise ...

I thought as much as well, but would it be nice to have for future support of other OS families?

Sure, but if we're not going to account for that in this PR, we should leave it out.

If we add future family support, I'd recommend doing two things:

Flag the resource for the additional family support using the supports method

Change the line to:

cmd = inspec.command(df_command_for_family)

... and supply a new df_command_for_family method that returns a command string appropriate for the node under test.

Signed-off-by: Vern Burton <[email protected]>

adamleff

Excellent work, @tarcinil!

adamleff · 2018-01-13T02:44:39Z

Hey @tarcinil - one last thing, and I'm sorry I didn't think about this sooner. I think we should rename this resource to filesystem. If we were to add Windows support in the future, those users wouldn't really understand why the resource is called df.

Also, if we switch to use a different method of inspecting filesystem properties other than the df command, it would be confusing.

What do you think?

tarcinil · 2018-01-15T17:08:48Z

@adamleff, I think that is a good argument. I will look at making the changes on Tuesday most likely.

tarcinil · 2018-01-23T17:29:27Z

I have time to fix this tonight. It has been a busy week between getting engaged and snowmageddon 2018.

adamleff · 2018-01-23T18:28:57Z

No problem, @tarcinil! Hope you're surviving the snow... no rush on this, and I thank you again for your contribution!

Signed-off-by: Vern Burton <[email protected]>

adamleff

Excellent work, @tarcinil! Thanks for introducing this new resource. I'm sure we'll be able to continue adding to it in the future.

chris-rock

Thank you @tarcinil

tarcinil added 6 commits December 30, 2017 14:56

Merge pull request #1 from chef/master

2a8bc9f

pulling chef:master

Merge branch 'master' of github.com:tarcinil/inspec

6fb3b6b

Merge remote-tracking branch 'upstream/master'

4b28aef

Merge remote-tracking branch 'upstream/master'

8aab4fe

adding df resource

3d6e64c

Signed-off-by: Vern Burton <[email protected]>

adding unit tests and required mocks for them, created integration test

3fb1356

Signed-off-by: Vern Burton <[email protected]>

tarcinil requested a review from a team as a code owner January 5, 2018 01:43

tarcinil added 3 commits January 4, 2018 19:52

cleaning up skip test to include only the filename and not full path

ca04c40

Signed-off-by: Vern Burton <[email protected]>

adding docs

bd0f763

Signed-off-by: Vern Burton <[email protected]>

size makes more sense than space

8eec1ae

Signed-off-by: Vern Burton <[email protected]>

adamleff suggested changes Jan 5, 2018

View reviewed changes

tarcinil added 4 commits January 10, 2018 22:17

removing unneeded author lines

2cf78f7

Signed-off-by: Vern Burton <[email protected]>

as the command changed, changing mock to the new sha

d28f2bd

Signed-off-by: Vern Burton <[email protected]>

adding supports to resource metadata, and adding tests that show that…

f5d3327

… resource says that it is not supported on windows/unix. Signed-off-by: Vern Burton <[email protected]>

adamleff suggested changes Jan 13, 2018

View reviewed changes

focusing on linux os family and removing logic for assumed future cases

9bc1ed6

Signed-off-by: Vern Burton <[email protected]>

adamleff approved these changes Jan 13, 2018

View reviewed changes

adamleff changed the title ~~Implement df resource~~ df resource: inspect disk space utilization on linux nodes Jan 13, 2018

adamleff added the Type: New Resource label Jan 13, 2018

adamleff changed the title ~~df resource: inspect disk space utilization on linux nodes~~ df resource: inspect linux filesystems Jan 13, 2018

tarcinil added 3 commits January 23, 2018 18:23

Merge remote-tracking branch 'upstream/master'

e0a4bff

Merge branch 'master' into feature/1831-df-resource

2c0c1bd

changing df to filesystem

d5fe2c4

Signed-off-by: Vern Burton <[email protected]>

adamleff approved these changes Jan 24, 2018

View reviewed changes

adamleff changed the title ~~df resource: inspect linux filesystems~~ filesystem resource: inspect linux filesystems Jan 24, 2018

chris-rock approved these changes Jan 25, 2018

View reviewed changes

adamleff added the Version: Bump Minor label Jan 25, 2018

adamleff merged commit 55abdeb into inspec:master Jan 25, 2018

zenspider added Expeditor: Bump Minor Version and removed Version: Bump Minor labels Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filesystem resource: inspect linux filesystems #2441

filesystem resource: inspect linux filesystems #2441

tarcinil commented Jan 5, 2018 •

edited

Loading

adamleff left a comment

adamleff Jan 5, 2018

adamleff Jan 5, 2018

adamleff Jan 5, 2018

tarcinil Jan 11, 2018

tarcinil Jan 11, 2018 •

edited

Loading

jquick Jan 11, 2018

adamleff Jan 5, 2018

adamleff Jan 5, 2018

adamleff Jan 5, 2018

tarcinil commented Jan 11, 2018

adamleff left a comment

adamleff Jan 13, 2018

tarcinil Jan 13, 2018

adamleff Jan 13, 2018

adamleff left a comment

adamleff commented Jan 13, 2018

tarcinil commented Jan 15, 2018

tarcinil commented Jan 23, 2018

adamleff commented Jan 23, 2018

adamleff left a comment

chris-rock left a comment

		@@ -0,0 +1,22 @@
		# encoding: utf-8
		# author: Christoph Hartmann

filesystem resource: inspect linux filesystems #2441

filesystem resource: inspect linux filesystems #2441

Conversation

tarcinil commented Jan 5, 2018 • 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

tarcinil Jan 11, 2018 • 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

tarcinil commented Jan 11, 2018

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

adamleff left a comment

Choose a reason for hiding this comment

adamleff commented Jan 13, 2018

tarcinil commented Jan 15, 2018

tarcinil commented Jan 23, 2018

adamleff commented Jan 23, 2018

adamleff left a comment

Choose a reason for hiding this comment

chris-rock left a comment

Choose a reason for hiding this comment

tarcinil commented Jan 5, 2018 •

edited

Loading

tarcinil Jan 11, 2018 •

edited

Loading