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.

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

harmonize profile targets #121

Merged
merged 3 commits into from
Oct 26, 2016
Merged

harmonize profile targets #121

merged 3 commits into from
Oct 26, 2016

Conversation

vjeffrey
Copy link

fixes: #118

unsure about the disabled part, looking into that to confirm that works..

@vjeffrey vjeffrey force-pushed the vj/harmonize-profile-targets branch from 054e28c to 859e1f5 Compare October 25, 2016 11:51
@chris-rock
Copy link
Contributor

Lets remove the disabled part, not sure why we introduced that in the first place. You can just remove the block for the profile location if you do not want to execute it. If I remember correctly that was intended to load profiles but do not report them. We do not need that anymore, since we will rely on InSpec core to handle that.


profiles.each { |profile|
if profile.include?(:supermarket_url)
profiles_array.push(profile[:supermarket_url] + "/" + profile[:name])
Copy link
Contributor

Choose a reason for hiding this comment

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

i think that will not work. we need to adapt the runner to give it more complex targets.


profiles.each { |profile|
if profile.length == 1 && profile[:name]
profiles_array.push("supermarket://" + profile[:name])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to give the runner the whole hash directly instead of transforming that to urls

Copy link
Contributor

Choose a reason for hiding this comment

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

we would need to adapt the add_target method in inspec: https://github.com/chef/inspec/blob/master/lib/inspec/runner.rb#L156-L164 to receive objects. By default it is using that function to change from url string to hash object: https://github.com/chef/inspec/blob/master/lib/inspec/profile.rb#L43-L46.

Instead we would invoke https://github.com/chef/inspec/blob/master/lib/inspec/profile.rb#L38-L41 from the runner initially. See how it is invoked at https://github.com/chef/inspec/blob/master/lib/inspec/dependencies/requirement.rb#L101-L106

@vjeffrey vjeffrey force-pushed the vj/harmonize-profile-targets branch 2 times, most recently from 3a39fdc to 78284c3 Compare October 26, 2016 03:03
@vjeffrey
Copy link
Author

vjeffrey commented Oct 26, 2016

tried to create test cookbook to include in examples folder for this, but having trouble.... Waiting for SSH service on 127.0.0.1:2200, retrying in 3 seconds i'll work on getting that in the morning...not sure what's happening there

Signed-off-by: Victoria Jeffrey <[email protected]>
@vjeffrey vjeffrey force-pushed the vj/harmonize-profile-targets branch from 78284c3 to 607db37 Compare October 26, 2016 03:10
# filesystem path
"source" => "E:/profiles/win2012_audit"
"source": "E:/profiles/win2012_audit"
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it needs to be path now

Copy link
Author

Choose a reason for hiding this comment

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

oh shoot, yes it does need to be path!

Signed-off-by: Victoria Jeffrey <[email protected]>
@vjeffrey vjeffrey force-pushed the vj/harmonize-profile-targets branch from cbf8300 to f7c2354 Compare October 26, 2016 11:51
Signed-off-by: Victoria Jeffrey <[email protected]>
@vjeffrey vjeffrey force-pushed the vj/harmonize-profile-targets branch from 9a9ff41 to 6a166f4 Compare October 26, 2016 12:53
@vjeffrey vjeffrey changed the title wip: harmonize profile targets harmonize profile targets Oct 26, 2016
@vjeffrey
Copy link
Author

@chris-rock: it's ready!

@vjeffrey vjeffrey changed the title harmonize profile targets harmonize profile targets Oct 26, 2016
@chris-rock
Copy link
Contributor

Thanks @vjeffrey for this great improvement

@chris-rock chris-rock merged commit 7df3601 into master Oct 26, 2016
@chris-rock chris-rock deleted the vj/harmonize-profile-targets branch October 26, 2016 13:33
@@ -2,3 +2,6 @@
source 'https://supermarket.chef.io'

metadata

cookbook 'apt'
cookbook 'yum'
Copy link
Contributor

@jeremymv2 jeremymv2 Oct 26, 2016

Choose a reason for hiding this comment

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

It looks like the 'git' client is just going to be used during testing/validation. In that case, the better pattern is to use a fixture cookbook to install git rather than a 'prep' recipe. See this example of installing 'java' for testing the 'jenkins' cookbook:
https://github.com/chef-cookbooks/jenkins/blob/master/Berksfile#L9-L21

https://github.com/chef-cookbooks/jenkins/blob/master/test/fixtures/cookbooks/jenkins_server_wrapper/recipes/default.rb#L3

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call @jeremymv2, agree on that! @vjeffrey lets do that

Copy link
Contributor

Choose a reason for hiding this comment

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

A simple/safe way of installing the git binary on linux is via the git cookbook:

https://github.com/chef-cookbooks/git/blob/master/recipes/default.rb

@@ -0,0 +1,2 @@
# this is required to run git dependencies via inspec
package 'git'
Copy link
Contributor

Choose a reason for hiding this comment

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

This recipe seems out of place if only for the purpose of being used during the testing phase in kitchen of this cookbook. In my opinion, a better approach is to install it via a fixture cookbook.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Implement RFC: Harmonize profile location targets
4 participants