New postgres_ident_conf resource

[aaronlippold]

This is a resource for parsing and processing the pg_ident.conf file for a postgres database.

[adamleff]

Hi @aaronlippold - thanks for your PR. I've supplied you some feedback to address, please. :)

[adamleff]

This is global feedback for this whole PR, putting it here since it made sense...

I think the resource name should be postgres_ident_conf... we should avoid abbreviations wherever possible, and we already have other PostgreSQL-related resources that are prefixed with postgres_

[adamleff]

Please remove this line.

[aaronlippold]

check. will do

[adamleff]

There's no need to set this variable that I can see, especially since you call inspec.postgres.conf_dir directly in the next line.

[adamleff]

@files_contents is set to an empty hash here, but later when the variable is populated, it's populated with an array. This should likely either be set to nil or an empty array rather than a hash. Ultimately, since this variable isn't really used anywhere, I'd eliminate it completely.

[aaronlippold]

agree we will change it to an array, i think it is just a hold over.

[adamleff]

I would reverse lines 28 and 29... don't try and read the file content on Windows if we're just going to skip_resource anyways.

[adamleff]

While newer versions of Ruby will also include tab characters as an empty space when splitting like this, it's probably more accurate (and clearer to the reader) to do line.split(/\s+/)

[adamleff]

I don't really see where this instance variable is used in a meaningful way, I think this whole method body can just be:

inspec.file(conf_file).content.lines

[adamleff]

This line, and 155, look like an error to me. Why would InSpec be trying to read in this directory? We shouldn't have to mock this.

[adamleff]

Also, since I can't add a review comment on an empty file, the mock files:

• test/unit/mock/files/var.9.5.main/.gitkeep
• test/unit/mock/files/9.5.main/.gitkeep

... shouldn't be necessary either. Let's fix whatever is causing you to add these to this PR.

[aaronlippold]

This was added to cover an issue with appvoior given that it is trying to run the unit tests on Windows and failing because we didn't mock for the windows side given that the skip resource was not being respected with FilterTable. This was a work around until we address the open issue #1965 .

[adamleff]

This is likely because of your direct call to inspec.postgres.conf_dir in the initialize method. Unit tests should not have to rely on OS or filesystem status as it's about testing the code/messages rather than a functional test.

You'll need to add a mock in your unit tests to cover this as adding these directory mockloader calls isn't appropriate.

It will look something like this (writing this off-the-cuff, haven't tested this):

describe 'blah' do
  let(:resource) { load_resource... }
  let(:inspec) { mock }
  let(:postgres) { mock }

  before do
    resource.stubs(:inspec).returns(inspec)
    inspec.stubs(:postgres).returns(postgres)
    postgres.stubs(:conf_dir).returns('/test/path/to/postgres')
  end

  it 'does something' do
    entries = resource.where ...
  end
end

This should stub/mock the call to inspec.postgres.conf_dir so that it always returns the string /test/path/to/postgres so it works regardless of operating system and whether or not PostgreSQL is installed on the machine performing the unit tests. And then I'd change the helper config to mock out /test/path/to/postgres/pg_ident.conf instead of the one in /etc/postgres... so that you're 100% certain your testing just your code, not some integration point on the system itself.

Search the tests in test/unit for additional examples of how to use mock and stubs. Let me know if you get hung up on this, but we have to square this away before this can be merged.

[adamleff]

Please remove this line.

[aaronlippold]

done.

[adamleff]

I'd like to see the tests account for a pg_ident file that has multiple entries to ensure the resource is parsing the file properly and handing back all the lines to FilterTable.

Just adding a second line and changing these must_match statements to match an array of contents would be sufficient for that.

[adamleff]

@aaronlippold and @rx294 this is getting closer. I've provided you the tip necessary to stub/mock the calls that are causing you Appveyor issues that we'll need to get squared away in order to eliminate the directory mocks you've added to the unit test helper.rb. Please let me know if you have any questions.

[adamleff]

This line, and line 56, are unnecessary. You already assign these to nil in the initialize method, and it's our mostly-standard practice to keep empty variables set to nil whenever possible.

[adamleff]

This is likely because of your direct call to inspec.postgres.conf_dir in the initialize method. Unit tests should not have to rely on OS or filesystem status as it's about testing the code/messages rather than a functional test.

You'll need to add a mock in your unit tests to cover this as adding these directory mockloader calls isn't appropriate.

It will look something like this (writing this off-the-cuff, haven't tested this):

describe 'blah' do
  let(:resource) { load_resource... }
  let(:inspec) { mock }
  let(:postgres) { mock }

  before do
    resource.stubs(:inspec).returns(inspec)
    inspec.stubs(:postgres).returns(postgres)
    postgres.stubs(:conf_dir).returns('/test/path/to/postgres')
  end

  it 'does something' do
    entries = resource.where ...
  end
end

This should stub/mock the call to inspec.postgres.conf_dir so that it always returns the string /test/path/to/postgres so it works regardless of operating system and whether or not PostgreSQL is installed on the machine performing the unit tests. And then I'd change the helper config to mock out /test/path/to/postgres/pg_ident.conf instead of the one in /etc/postgres... so that you're 100% certain your testing just your code, not some integration point on the system itself.

Search the tests in test/unit for additional examples of how to use mock and stubs. Let me know if you get hung up on this, but we have to square this away before this can be merged.

[adamleff]

While these are fine tests, this isn't exactly what I was trying to convey with my earlier review. 🙂 Your tests are testing more the .where functionality of FilterTable than you are your parsing engine.

A better more-slimmed-down test might look like this:

it 'parses the pg_ident.conf file correctly' do
  resource.map_name.must_equal ['omicron', 'ssl-test', 'pki-users']
  resource.system_username.must_equal ['bryanh', 'ann', 'robert']
  resource.pg_username.must_equal ['bryanh', 'ann', 'bob']
end

This eliminates any unnecessary calls to FilterTable and focuses solely on this resource's ability to parse your file and populate the FilterTable itself.

You also don't need the two describe statements here... just whittle it down to one and make it match the actual class name of the resource without quotes:

describe Inspec::Resources::PostgresIdentConf do
  it 'parses the pg_ident.conf file correctly' do
    # stuff here
  end
end

Additional describe statements can always be added later to cover individual helper methods, etc.

[rx294]

I have made the corrections and added the tests.Please review them when you get a chance. Thank you again @adamleff !

[adamleff]

This looks good - thanks @aaronlippold and @rx294!

[chris-rock]

Awesome addition @rx294 and @aaronlippold I just have a super mini change-request. Once done, LGTM

[chris-rock]

All files will be apache licensed, lets remove the empty copyright here

[aaronlippold]

@chris-rock changes made.