support new automate compliance backend #1819

chris-rock · 2017-05-16T22:45:30Z

No description provided.

adamleff

I don't think any of my comments are blocking, but my opinion is that they'd make the code cleaner and easier to test if implemented.

adamleff · 2017-05-17T02:46:09Z

lib/bundles/inspec-compliance/api.rb

+          mapped_profiles = profiles.values.flatten
+        else
+          owner_id = config['user']
+          mapped_profiles = profiles.map { |e|


Stylistically, we should ensure multiline blocks be of the do...end variety to remain consistent.

adamleff · 2017-05-17T02:49:23Z

lib/bundles/inspec-compliance/api.rb

-        if config['server_type'] == 'automate'
-          mapped_profiles = profiles.values.to_a.flatten
-        else
+        if config['server_type'] == 'compliance'


We're doing this if... elsif...else expressions a few times throughout this change. Can I propose some helper methods?

def is_compliance_server?(config) config['server_type'] == 'compliance' end def is_automate_server_pre_080?(config) config['server_type'] == 'automate' && config['version'].empty? end def is_automate_server_080_and_later?(config) config['server_type'] == 'automate' && !config['version'].empty? end def is_automate_server?(config) config['server_type'] == 'automate' end

That way we're only defining the logic in one place and it's easier to add unit tests.

We can definitely do that. Even after my fix, this module needs massive refactoring. It started with 1 server version, and we have essentially 2 versions Chef Compliance and 2 versions Chef Automate now. That needs to be abstracted.

adamleff · 2017-05-17T02:50:58Z

lib/bundles/inspec-compliance/api.rb

+    def self.version(config)
+      url = config['server']
+      insecure = config['insecure']
+
      if url.nil?
        puts "
 Server configuration information is missing.
 Please login using `inspec compliance login https://compliance.test --user admin --insecure --token 'PASTE TOKEN HERE' `
 "


I know this line isn't changed in your PR, but shouldn't we be exiting here if we don't have the config? We seem to continue on, and it looks like data will never be an assigned variable and we'll likely throw an exception.

if url.nil? puts 'warning message' exit 1 end # reach out to compliance API as needed

API implementation may throw an error but will never exit, this can only be done by CLI

adamleff · 2017-05-17T02:51:37Z

lib/bundles/inspec-compliance/api.rb

-      # upload the tar to Chef Compliance
-      config['server_type'] == 'automate' ? url = "#{config['server']}/#{config['user']}" : url = "#{config['server']}/owners/#{owner}/compliance/#{profile_name}/tar"
+      # Chef Compliance
+      if config['server_type'] == 'compliance'


See earlier comment about using helper methods here instead of repeating the config[] logic.

adamleff · 2017-05-17T02:52:49Z

lib/bundles/inspec-compliance/api.rb

@@ -151,12 +178,23 @@ def self.get_token(config)

    def self.target_url(config, profile)
      if config['server_type'] == 'automate'
-        target = "#{config['server']}/#{profile}/tar"
+        owner, id = profile.split('/')


The splitting of the profile into owner and ID is now redundantly defined. We could eliminate the redundancy:

owner, id = profile.split('/') if is_automate_server?(config) # etc...

adamleff · 2017-05-17T03:00:10Z

lib/bundles/inspec-compliance/api.rb

      else
        owner, id = profile.split('/')
        target = "#{config['server']}/owners/#{owner}/compliance/#{id}/tar"
      end
      target
    end
+
+    # returns a parsed url for `admin/profile` or `compliance://admin/profile`
+    def self.sanitize_profile_name(profile)


I'm sure I'm missing something with this method... The end state is to return 'admin/profile' regardless of whether it was supplied with a URI scheme or not, correct? It seems like we don't need to prepend a non-schemed URI with a scheme just to remove the scheme from the string. In fact, that's kinda what was being done in an earlier version of this method is removed later in this PR.

Could this method be simplified to:

uri = URI(profile) "#{uri.host}#{uri.path}"

irb(main):021:0* a = URI('compliance://admin/profile') => #<URI::Generic:0x007fc51b0b1980 URL:compliance://admin/profile> irb(main):022:0> b = URI('admin/profile') => #<URI::Generic:0x007fc51b0a8420 URL:admin/profile> irb(main):023:0> "#{a.host}#{a.path}" => "admin/profile" irb(main):024:0> "#{b.host}#{b.path}" => "admin/profile"

Like I said, I'm sure I'm missing something here so feel free to educate me!

I just learned yesterday that Automate is using fancy usernames like adam@leff.com, which breaks the current parsing since adam@ is not part of host. I agree that we need way more testing around this

Ah, I understand. I'd love for us to use URI smarter than just deleting the scheme from the string, but we can tackle that another time.

adamleff · 2017-05-17T03:03:31Z

lib/bundles/inspec-compliance/cli.rb

-      if config['server_type'] == 'automate'
-        puts 'Version not available when logged in with Automate.'
+      info = Compliance::API.version(config)
+      if !info.nil? && info['version']


It might be nice to have Compliance::API.version return an empty hash instead of nil to avoid having to do this extra check.

I like that

arlimus · 2017-05-17T10:18:57Z

lib/bundles/inspec-compliance/api.rb

+      # Chef Automate
+      elsif is_automate_server?(config)
+        url = "#{config['server']}/profiles/#{config['user']}"
+      end


Let's catch the third case i.e. else raise ... becuse there is no url instead of getting the nil error further down

arlimus

Thank you Chris for adding all of this!
Ditto Adam for the review, I agree this needs more work.
My comment isn't critical for the merge, but needs to land at some point (afaics).

chris-rock · 2017-05-17T10:24:35Z

@arlimus will add that quickly...

adamleff · 2017-05-17T11:24:55Z

lib/bundles/inspec-compliance/api.rb

+      elsif is_automate_server?(config)
+        url = "#{config['server']}/profiles/#{config['user']}"
+      else
+        raise ServerConfigurationMissing


I'd just add a string message to this to make it complete and give the troubleshooter an additional clue.

raise ServerConfigurationMissing, 'No `server_type` entry in the config, unable to determine server type.'

adamleff · 2017-05-17T11:25:53Z

lib/bundles/inspec-compliance/api.rb

+      url = config['server']
+      insecure = config['insecure']
+
+      raise ServerConfigurationMissing if url.nil?


Same here...

raise ServerConfigurationMissing, 'No `server` entry in config, unable to determine server URL.' if url.nil?

adamleff

I'd obviously love to see some unit tests for this bundle but understand that time is not on our side. At least factoring out some of the logic into helper methods will make it easier to do so in the future.

Thanks for fixing this all up, @chris-rock.

adamleff · 2017-05-17T11:29:07Z

@chris-rock don't forget to fix up the DCO issue with your third commit before we merge.

chris-rock · 2017-05-17T11:38:21Z

Once @alexpop approves, I squash the commits

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

alexpop

Works great, thanks Chris!

chris-rock requested a review from alexpop May 16, 2017 22:45

chris-rock added the in progress label May 16, 2017

chris-rock force-pushed the chris-rock/compliance-plugin branch from 42bd079 to 035281c Compare May 16, 2017 22:45

adamleff reviewed May 17, 2017

View reviewed changes

chris-rock force-pushed the chris-rock/compliance-plugin branch from 569a3eb to 88349d9 Compare May 17, 2017 08:53

arlimus reviewed May 17, 2017

View reviewed changes

arlimus approved these changes May 17, 2017

View reviewed changes

adamleff reviewed May 17, 2017

View reviewed changes

adamleff approved these changes May 17, 2017

View reviewed changes

support new automate 0.8.0

5cc288d

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

chris-rock force-pushed the chris-rock/compliance-plugin branch from 20a65b3 to 5cc288d Compare May 17, 2017 13:02

alexpop approved these changes May 17, 2017

View reviewed changes

chris-rock merged commit 3140eea into master May 17, 2017

chris-rock deleted the chris-rock/compliance-plugin branch May 17, 2017 13:15

chris-rock removed the in progress label May 17, 2017

adamleff added the Type: Enhancement Improves an existing feature label May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support new automate compliance backend #1819

support new automate compliance backend #1819

chris-rock commented May 16, 2017

adamleff left a comment

adamleff May 17, 2017

adamleff May 17, 2017

chris-rock May 17, 2017

adamleff May 17, 2017

chris-rock May 17, 2017

adamleff May 17, 2017

adamleff May 17, 2017

adamleff May 17, 2017

chris-rock May 17, 2017

adamleff May 17, 2017

adamleff May 17, 2017

chris-rock May 17, 2017 •

edited

Loading

arlimus May 17, 2017

chris-rock May 17, 2017

arlimus left a comment

chris-rock commented May 17, 2017

adamleff May 17, 2017

adamleff May 17, 2017

adamleff left a comment

adamleff commented May 17, 2017

chris-rock commented May 17, 2017

alexpop left a comment

support new automate compliance backend #1819

support new automate compliance backend #1819

Conversation

chris-rock commented May 16, 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

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

chris-rock May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

chris-rock commented May 17, 2017

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 May 17, 2017

chris-rock commented May 17, 2017

alexpop left a comment

Choose a reason for hiding this comment

chris-rock May 17, 2017 •

edited

Loading