Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support profile versions for automate profiles storage #2128

Merged
merged 3 commits into from
Sep 13, 2017
Merged

Support profile versions for automate profiles storage #2128

merged 3 commits into from
Sep 13, 2017

Conversation

alexpop
Copy link
Contributor

@alexpop alexpop commented Sep 8, 2017

After inspec compliance login_automate ...

$ be inspec compliance exec compliance://admin/apache-baseline#2.0.1

Profile: DevSec Apache Baseline (apache-baseline)
Version: 2.0.1
...

$ be inspec compliance exec admin/apache-baseline

Profile: DevSec Apache Baseline (apache-baseline)
Version: 2.0.2

$ be inspec exec compliance://admin/apache-baseline#2.0.1

Profile: DevSec Apache Baseline (apache-baseline)
Version: 2.0.1

$ be inspec exec compliance://admin/apache-baseline

Profile: DevSec Apache Baseline (apache-baseline)
Version: 2.0.2

$ be inspec compliance download compliance://admin/apache-baseline#2.0.1
Downloading `admin/apache-baseline#2.0.1`
Profile stored to apache-baseline#2.0.1.tar.gz
$ inspec json apache-baseline#2.0.1.tar.gz | jq .version
"2.0.1"

@alexpop alexpop self-assigned this Sep 8, 2017
@alexpop alexpop requested a review from a team as a code owner September 8, 2017 14:15
adamleff
adamleff suggested changes Sep 8, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks fine overall. Can we please add some unit tests for .target_url and .profile_split? I'd like to see both tests for profile names that include and don't include a version string to prevent future regressions.

lib/bundles/inspec-compliance/api.rb Outdated
if !profiles.empty?
index = profiles.index { |p| "#{p['owner_id']}/#{p['name']}" == profile }
index = profiles.index { |p|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiline blocks should use do..end please :)

lib/bundles/inspec-compliance/api.rb Outdated
if !profiles.empty?
index = profiles.index { |p| "#{p['owner_id']}/#{p['name']}" == profile }
index = profiles.index { |p|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never really use the index except for checking to see if we have a match. Could we refactor this to use #any? instead while we're in here?

profiles.any? do |p|
  # criteria
end

... that should return true if there was a match, false if not.

@adamleff
Copy link
Contributor

adamleff commented Sep 8, 2017

We also need to update the inspec-compliance README since this is a new enhancement.

@alexpop
Copy link
Contributor Author

alexpop commented Sep 8, 2017

Makes sense Adam, appreciate the review. Will return next week and finish this up.

@alexpop alexpop force-pushed the ap-vj/compliance-profiles-ver branch 2 times, most recently from 2c38fcc to ed3cc01 Compare September 13, 2017 16:08
@alexpop
Copy link
Contributor Author

alexpop commented Sep 13, 2017

Adam, I added a second commit to the PR that:

  • addresses the code refactor you mentioned
  • updates the inspec-compliance bundle README
  • adds tests (Wasn't able to stub the profiles method in the exist? test. Can you have a look at that please?)

Cheers!

@adamleff
Copy link
Contributor

@alexpop I think I squared away the pending errors and did a little refactor of one method. Please check 33f57aa and see if that meets your needs. The tests should go green shortly.

adamleff
adamleff approved these changes Sep 13, 2017
View reviewed changes
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with my clean-up commit. @alexpop please confirm!

@alexpop alexpop force-pushed the ap-vj/compliance-profiles-ver branch from 33f57aa to f766d9c Compare September 13, 2017 20:16
@alexpop
Copy link
Contributor Author

alexpop commented Sep 13, 2017

Looks very good Adam, happy to merge!

@adamleff adamleff added the Type: Enhancement Improves an existing feature label Sep 13, 2017
@adamleff adamleff merged commit 35becd7 into master Sep 13, 2017
@adamleff adamleff deleted the ap-vj/compliance-profiles-ver branch September 13, 2017 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamleff adamleff adamleff approved these changes

@jerryaldrichiii jerryaldrichiii Awaiting requested review from jerryaldrichiii

Assignees

@alexpop alexpop

Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexpop @adamleff