updated the psql command so that it does not print the headers and ex… #1714

aaronlippold · 2017-04-25T23:51:30Z

…tra output

adamleff

Hi @aaronlippold and thanks for your contribution.

I'm concerned that this change is not technically backward compatible with existing versions and users of InSpec 1.x. If users are using this resource and expecting the output to not be stripped and not expecting that the spaces before and after the field separator to be missing, we can potentially break their existing tests. I would definitely flag this change as a change acceptable for InSpec v2.

A potential workaround is to introduce a new rows_from_query method that takes a query and formats it as an array of hashes, each row being an a hash in that array. That way the query method return format remains unaltered.

Also, you'll need to sign-off each of your commits in accordance with the DCO.

@arlimus and @chris-rock, your feedback requested on the backward compatibility aspect of this change.

adamleff · 2017-04-26T16:32:54Z

lib/resources/postgres_session.rb

      out = cmd.stdout + "\n" + cmd.stderr
      if cmd.exit_status != 0 or
         out =~ /could not connect to .*/ or
         out.downcase =~ /^error/
        # skip this test if the server can't run the query
        skip_resource "Can't read run query #{query.inspect} on postgres_session: #{out}"
      else
+        Lines.new(cmd.stdout.strip, "PostgreSQL query: #{query}")
+        # @note the below should not be needed now that you are passing -A -t to psql


Let's delete these comments from lines 60-65... they're not needed anymore.

adamleff · 2017-04-26T16:34:16Z

lib/resources/postgres_session.rb

      end
+      alias_method psql query


Let's not create aliases unless they are absolutely necessary. Commands like SHOW port are still queries and can be handled with the query method.

adamleff · 2017-04-26T16:34:54Z

lib/resources/postgres_session.rb

@@ -31,6 +31,10 @@ class PostgresSession < Inspec.resource(1)
      describe sql.query('SELECT * FROM pg_shadow WHERE passwd IS NULL;') do
        its('output') { should eq('') }
      end
+
+      describe sql.psql('SHOW port') do


I do not agree with creating a psql method. No matter how the query is performed, even SHOW port is still a query so the query method will suffice.

aaronlippold · 2017-05-03T20:52:30Z

@adamleff the change specifically for the extra command flags to the psql command do not change how the function operates at all it just removes the need to have the extra substring munging to remove text that was and has been in the codebase.

 -                   .sub(/(.*\n)+([-]+[+])*[-]+\n/, '')		
 -                   .sub(/\n[^\n]*\n\n$/, '')```

I just had psql do that for us rather than doing it ourselves.

* Alias function removed
*  Formatting fixed
* Reviewed by secondary source

arlimus · 2017-05-09T14:32:21Z

Tracked down psql a bit for the added commandline options; Afaics 7.0 had -A -t included too (since 2000). Thank you for raising this, it's good that we know about these things and we need to include them in these resources when this part of the platforms RFC lands (#1661)

arlimus · 2017-05-09T14:34:45Z

lib/resources/postgres_session.rb

+      sql = postgres_session('username', 'password', 'host')
+
+      query(string): required
+      db(array[string]): optional, defaults to `username == db`


Small suggestion:

sql = postgres_session('username', 'password', 'host') # with default values: # username: 'postgres' # host: 'localhost'

arlimus · 2017-05-09T14:38:44Z

Thank you for the updates @aaronlippold
Kudos for the review @adamleff
👍

Once the example is updated this looks good from my perspective

arlimus

Great updates thank you!

adamleff

One small thing and then I'm OK to approve.

adamleff · 2017-05-10T16:34:44Z

lib/resources/postgres_session.rb


      describe sql.query('SELECT * FROM pg_shadow WHERE passwd IS NULL;') do
        its('output') { should eq('') }
      end
    "

-    def initialize(user, pass)
+    def initialize(user, pass, host)


This isn't backward compatible with the existing resource. We should probably change this to:

def initialize(user, pass, host=nil)

Otherwise, anyone already using a postgres_session resource will get:

ArgumentError: wrong number of arguments (given 2, expected 3)

Great catch @adamleff ! I missed that

@adamleff @arlimus would we want to make it nil or localhost, if we make it localhost as the function header default then we could remove L46?

Ok. Made the change, I guess it doesn't matter what is in the method signature given that the constructer sets the value right away anyway. Changes pushed.

aaronlippold · 2017-05-11T12:30:13Z

lib/resources/postgres_session.rb

      @user = user || 'postgres'
      @pass = pass
+      @host = host || 'localhost'


i.e. here...

Signed-off-by: Aaron Lippold <lippold@gmail.com>

adamhjk added the in progress label Apr 25, 2017

adamleff suggested changes Apr 26, 2017

View reviewed changes

aaronlippold force-pushed the postgres-session-update branch 5 times, most recently from c2e39b3 to 0c9d31e Compare May 3, 2017 19:17

arlimus reviewed May 9, 2017

View reviewed changes

aaronlippold force-pushed the postgres-session-update branch 2 times, most recently from 03f6e0d to 82d5702 Compare May 9, 2017 16:01

arlimus approved these changes May 9, 2017

View reviewed changes

adamleff suggested changes May 10, 2017

View reviewed changes

aaronlippold commented May 11, 2017

View reviewed changes

aaronlippold force-pushed the postgres-session-update branch from 82d5702 to 9d1d025 Compare May 11, 2017 12:53

psql doesn't print headers + extra output + cconfigurable host + docs

684d81d

Signed-off-by: Aaron Lippold <lippold@gmail.com>

aaronlippold force-pushed the postgres-session-update branch from 9d1d025 to 684d81d Compare May 11, 2017 12:59

adamleff approved these changes May 11, 2017

View reviewed changes

adamleff merged commit a5e5cc0 into inspec:master May 11, 2017

adamleff removed the in progress label May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated the psql command so that it does not print the headers and ex… #1714

updated the psql command so that it does not print the headers and ex… #1714

aaronlippold commented Apr 25, 2017

adamleff left a comment

adamleff Apr 26, 2017

adamleff Apr 26, 2017

adamleff Apr 26, 2017

aaronlippold commented May 3, 2017

arlimus commented May 9, 2017

arlimus May 9, 2017

arlimus commented May 9, 2017

arlimus left a comment

adamleff left a comment

adamleff May 10, 2017

arlimus May 10, 2017 •

edited

Loading

aaronlippold May 11, 2017

aaronlippold May 11, 2017

aaronlippold May 11, 2017

updated the psql command so that it does not print the headers and ex… #1714

updated the psql command so that it does not print the headers and ex… #1714

Conversation

aaronlippold commented Apr 25, 2017

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

aaronlippold commented May 3, 2017

arlimus commented May 9, 2017

Choose a reason for hiding this comment

arlimus commented May 9, 2017

arlimus left a 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

arlimus May 10, 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

arlimus May 10, 2017 •

edited

Loading