etc_fstab resource: test contents of the /etc/fstab file #2064

dromazmj · 2017-08-14T12:42:15Z

Signed-off-by: dromazos [email protected]

Signed-off-by: dromazos <[email protected]>

adamleff

Hi @dromazmj - thanks for your PR. I've left you some feedback. Please let me know if you have any questions.

adamleff · 2017-08-17T18:02:03Z

docs/resources/etc_fstab.md.erb

+Use the where clause to match a property to one or more rules in the fstab file.
+
+    describe etc_fstab.where { device_name == 'value' } do
+      its ( 'mount_point' ) { should eq ['hostname'] }


Can we change this to use the cmp matcher? should cmp 'hostname' since the cmp matcher will strip a single array item out of the Array.

adamleff · 2017-08-17T18:02:27Z

docs/resources/etc_fstab.md.erb

+
+    describe etc_fstab.where { device_name == 'value' } do
+      its ( 'mount_point' ) { should eq ['hostname'] }
+      its ( 'file_system_type' ) { should eq [['list']] }


Can we change all of these to be cmp matchers as well? Shouldn't need the double-array treatment.

adamleff · 2017-08-17T18:02:42Z

docs/resources/etc_fstab.md.erb

+Use the optional constructor parameter to give an alternative path to fstab file
+
+    describe etc_fstab(hosts_path).where { device_name == 'value' } do
+      its ( 'mount_point' ) { should eq ['hostname'] }


Same here re: cmp matcher instead of the eq matcher

adamleff · 2017-08-17T18:03:35Z

docs/resources/etc_fstab.md.erb

+where
+
+* `device_name` is the name associated with the device.
+* `mount_point` is the default directory the device will be mounted if one is not specified in the mount command.


Rather than talk about how the mount command works in its ability to override the fstab, how about:

"mount_point is the directory at which the filesystem is configured to be mounted"

adamleff · 2017-08-17T18:08:47Z

docs/resources/etc_fstab.md.erb

+
+### device_name(String)
+
+`device_name` returns an array of strings that matches the where condition of the filter table.


You don't have to use the where condition to write effective tests with FilterTable, so can we change the documentation of each of these examples to eliminate the "where condition of the filter table"?

End-users don't need to know what a FilterTable is and we don't really define it anywhere in the docs either. Let's keep it focused on a great user experience.

Also, for all of these examples, especially ones that return an array that is likely to be 1 item, let's switch to the cmp matcher please and get rid of the unnecessary Array syntax.

adamleff · 2017-08-17T18:21:47Z

lib/resources/etc_fstab.rb

+    where { mount_point == '/home' }.entries[0].mount_options
+  end
+
+  def mounted?(point)


Also, I think this is a bit of a misleading method name... this isn't actually testing if it's mounted, but rather configured to be mounted.

I think configured? might be a better method name. If so, we will need to update the examples/documentation as well. I'm open to other suggestions, but it's totally possible to have something in /etc/fstab that's not actually mounted.

adamleff · 2017-08-17T18:23:19Z

lib/resources/etc_fstab.rb

+
+    raw_conf = file.content
+    if raw_conf.empty? && !file.empty?
+      return skip_resource("File is empty. If this is the correct file,


This skip_resource string is misleading. The file is specifically NOT empty but we couldn't read it. Let's remove reference to "access control" and instead make this something like:

"Unable to read file - check file permissions."

adamleff · 2017-08-17T18:23:45Z

lib/resources/etc_fstab.rb

+  def read_file(conf_path = @conf_path)
+    file = inspec.file(conf_path)
+    if !file.file?
+      return skip_resource "Can't find file. If this is the correct path,


Please remove reference to access control.

"Cannot find file #{conf_path}" is a sufficient error message.

adamleff · 2017-08-17T18:27:11Z

test/unit/resources/etc_fstab_test.rb

+    _(entries.file_system_options).must_equal [0]
+  end
+
+  it 'Verify etc_hosts filtering by `mount_point`'  do


I actually don't think you need this test, of the tests starting at lines 27, 36, 45, or 57. These tests are all fundamentally testing FilterTable, not really testing your custom resource.

These are the tests I would want to see:

basic support of parsing the file

parsing an entry where mount_options is a single item

parsing an entry where mount_options has multiple items.

test that home_mount_options returns something when there is a /home mount configured

test that home_mount_options returns nil when there isn't a /home mount configured

that that nfs_file_systems returns the correct entries

adamleff · 2017-08-17T18:27:37Z

test/unit/resources/etc_fstab_test.rb

+    _(entries.dump_options).must_equal [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+  end
+
+  it 'verify etc_fstab can detect if a partition is mounted' do


This isn't actually testing anything - it's creating a stub handler but it's never used.

Signed-off-by: dromazos <[email protected]>

adamleff

This is super-close, @dromazmj! Just a bit of cleanup and then I think it's ready for final review.

adamleff · 2017-08-25T13:41:37Z

docs/resources/etc_fstab.md.erb

+Use the where clause to match a property to one or more rules in the fstab file.
+
+    describe etc_fstab.where { device_name == 'value' } do
+      its ( 'mount_point' ) { should cmp 'hostname' }


For consistent Ruby style, please update these examples so that there are only spaces between the its parameter and the beginning of the block, as well as inside the block:

its('foo') { should cmp 'foo' }

Please make that change across all your pending PRs and the rest of the examples in this resource. Thank you!

adamleff · 2017-08-25T13:43:28Z

docs/resources/etc_fstab.md.erb

+### Check if a partition is mounted at a point.
+
+    describe etc_fstab.where { mount_point == '/home' } do
+      it { should be_mounted }


This should be be_configured now, right?

dromazmj · 2017-08-25T17:49:43Z

Awesome I'll have it to you by the end of the weekend!

-Matthew Dromazos Get Outlook for Android<https://aka.ms/ghei36>

________________________________ From: Adam Leff <[email protected]> Sent: Friday, August 25, 2017 12:01:18 PM To: chef/inspec Cc: Dromazos, Matthew James - dromazmj; Mention Subject: Re: [chef/inspec] etc_fstab resource: test contents of the /etc/fstab file (#2064) @adamleff requested changes on this pull request. This is super-close, @dromazmj<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_dromazmj&d=DwMCaQ&c=RWKzQL7PIkYKzsYSjZn54yrN72ooGPhHwUXdJ0S2Irw&r=Ko-0czWtwYWQx5BsaYOWL30uwy-xPDyvngxpH7Hzyls&m=uyMy9SM6zxdt5AJBUbob3RDKivja06J2u_CXimFe0xI&s=D3KPVPHERTfyi3dr_uk6_snSyVTWEeh3FqAd9XM5bV4&e=>! Just a bit of cleanup and then I think it's ready for final review.

________________________________ In docs/resources/etc_fstab.md.erb<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_chef_inspec_pull_2064-23discussion-5Fr135261121&d=DwMCaQ&c=RWKzQL7PIkYKzsYSjZn54yrN72ooGPhHwUXdJ0S2Irw&r=Ko-0czWtwYWQx5BsaYOWL30uwy-xPDyvngxpH7Hzyls&m=uyMy9SM6zxdt5AJBUbob3RDKivja06J2u_CXimFe0xI&s=n_e8lDfPX_WB1HuWSfZRqVMoVOUQDuOosLasStdQw9A&e=>:

+---

+ +# etc_fstab + +Use the `etc_fstab` InSpec audit resource to test information about all partitions and storage devices on a system. +## Syntax + +An etc_fstab rule specifies a device name, its mount point, its mount type, the options its mounted with, +its dump options, and the order the files system should be checked. + +## Syntax + +Use the where clause to match a property to one or more rules in the fstab file. + + describe etc_fstab.where { device_name == 'value' } do + its ( 'mount_point' ) { should cmp 'hostname' } For consistent Ruby style, please update these examples so that there are only spaces between the its parameter and the beginning of the block, as well as inside the block: its('foo') { should cmp 'foo' } Please make that change across all your pending PRs and the rest of the examples in this resource. Thank you!

________________________________ In docs/resources/etc_fstab.md.erb<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_chef_inspec_pull_2064-23discussion-5Fr135261522&d=DwMCaQ&c=RWKzQL7PIkYKzsYSjZn54yrN72ooGPhHwUXdJ0S2Irw&r=Ko-0czWtwYWQx5BsaYOWL30uwy-xPDyvngxpH7Hzyls&m=uyMy9SM6zxdt5AJBUbob3RDKivja06J2u_CXimFe0xI&s=p2eDmOk2u2DBpsm4R8ysPUOksCnUFVP5lHsNRnBxkYo&e=>:

+ nfs_systems.each do |partition|

+ describe partition do + its ('mount_options') { should include 'nosuid' } + end + end + +### Check the partition mounted at /home contains 'nosuid' in its mount_options. + + describe etc_fstab do + its ('home_mount_options') { should include 'nosuid' } + end + +### Check if a partition is mounted at a point. + + describe etc_fstab.where { mount_point == '/home' } do + it { should be_mounted } This should be be_configured now, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_chef_inspec_pull_2064-23pullrequestreview-2D58655912&d=DwMCaQ&c=RWKzQL7PIkYKzsYSjZn54yrN72ooGPhHwUXdJ0S2Irw&r=Ko-0czWtwYWQx5BsaYOWL30uwy-xPDyvngxpH7Hzyls&m=uyMy9SM6zxdt5AJBUbob3RDKivja06J2u_CXimFe0xI&s=xQyq-KPBgcNSAi6Qz8QGUK9l3zMO7KU73fdVAv2qtlo&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AbYyTTXJ5FLp4ulHLoZd36NT2uLPUIobks5sbu-5FOgaJpZM4O2UPW&d=DwMCaQ&c=RWKzQL7PIkYKzsYSjZn54yrN72ooGPhHwUXdJ0S2Irw&r=Ko-0czWtwYWQx5BsaYOWL30uwy-xPDyvngxpH7Hzyls&m=uyMy9SM6zxdt5AJBUbob3RDKivja06J2u_CXimFe0xI&s=uyppB2zh3zH-K2Bm2rMsl8UqdHEjPdObRO0ZwR8zrqo&e=>.

Signed-off-by: dromazmj <[email protected]>

Signed-off-by: Adam Leff <[email protected]>

Signed-off-by: Adam Leff <[email protected]> Signed-off-by: dromazmj <[email protected]>

…into al/etc_fstab Signed-off-by: dromazmj <[email protected]>

Signed-off-by: dromazmj <[email protected]>

adamleff

This looks to be in good shape. Thanks, @dromazmj!

arlimus

Amazing new resource, thank you so much @dromazmj !!

arlimus · 2017-09-11T13:55:19Z

Thank you for guiding this through @adamleff 😄

This commit/PR is to re-run the Expeditor actions for #2064 (changelog, automated build), etc. While we would normally manually re-run the actions through our bot, the expeditor config in the squashed commit for #2064 is not correct. Signed-off-by: Adam Leff <[email protected]>

dromazmj added 3 commits August 14, 2017 08:36

Adding support for fstab

8946935

Signed-off-by: dromazos <[email protected]>

New Resource - etc_fstab

0b71f0f

Signed-off-by: dromazos <[email protected]>

New Resource - etc_fstab

5de7643

Signed-off-by: dromazos <[email protected]>

adamleff changed the title ~~New Resource - etc_fstab~~ etc_fstab resource: test contents of the /etc/fstab file Aug 17, 2017

adamleff suggested changes Aug 17, 2017

View reviewed changes

adamleff mentioned this pull request Aug 17, 2017

etc_hosts resource: test the contents of the /etc/hosts file #2065

Merged

adamleff added the Type: New Resource label Aug 17, 2017

adamleff mentioned this pull request Aug 17, 2017

etc_hosts_allow and etc_hosts_deny resources: test the content of the tcpwrappers configuration files #2073

Merged

Modifications to new resource - etc_fstab

d1fb40b

Signed-off-by: dromazos <[email protected]>

dromazmj requested a review from a team as a code owner August 18, 2017 14:26

dromazmj added 2 commits August 18, 2017 10:27

Modifications to new resource - etc_fstab

e30de4b

Signed-off-by: dromazos <[email protected]>

Modifications to new resource - etc_fstab

20ad8fe

Signed-off-by: dromazos <[email protected]>

adamleff suggested changes Aug 25, 2017

View reviewed changes

dromazmj and others added 2 commits August 27, 2017 22:16

Modifications to docs of new resource etc_fstab

dd239ed

Signed-off-by: dromazmj <[email protected]>

Fix conflict on test/helper.rb

d74f18f

Signed-off-by: Adam Leff <[email protected]>

adamleff force-pushed the al/etc_fstab branch from 0396f60 to d74f18f Compare August 30, 2017 17:26

dromazmj force-pushed the al/etc_fstab branch from 51248a5 to d74f18f Compare September 5, 2017 14:43

Merge branch 'master' into al/etc_fstab

2bc3dc2

dromazmj closed this Sep 5, 2017

Fix conflict on test/helper.rb

cb71734

Signed-off-by: Adam Leff <[email protected]> Signed-off-by: dromazmj <[email protected]>

dromazmj reopened this Sep 5, 2017

Merge branch 'al/etc_fstab' of https://github.com/aaronlippold/inspec …

994d3da

…into al/etc_fstab Signed-off-by: dromazmj <[email protected]>

dromazmj force-pushed the al/etc_fstab branch from 382a733 to 994d3da Compare September 5, 2017 17:30

Modifications to new resource etc_fstab

fb8ab20

Signed-off-by: dromazmj <[email protected]>

adamleff approved these changes Sep 7, 2017

View reviewed changes

arlimus approved these changes Sep 11, 2017

View reviewed changes

arlimus merged commit 70548ab into inspec:master Sep 11, 2017

adamleff mentioned this pull request Sep 11, 2017

etc_fstab resource: test contents of the /etc/fstab file #2131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etc_fstab resource: test contents of the /etc/fstab file #2064

etc_fstab resource: test contents of the /etc/fstab file #2064

dromazmj commented Aug 14, 2017 •

edited

Loading

adamleff left a comment

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff Aug 17, 2017

adamleff left a comment

adamleff Aug 25, 2017

adamleff Aug 25, 2017

dromazmj commented Aug 25, 2017 via email

adamleff left a comment

arlimus left a comment

arlimus commented Sep 11, 2017


		### device_name(String)

		`device_name` returns an array of strings that matches the where condition of the filter table.

etc_fstab resource: test contents of the /etc/fstab file #2064

etc_fstab resource: test contents of the /etc/fstab file #2064

Conversation

dromazmj commented Aug 14, 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

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

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

dromazmj commented Aug 25, 2017 via email

adamleff left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

arlimus commented Sep 11, 2017

dromazmj commented Aug 14, 2017 •

edited

Loading