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

Merged interval functionality into default.rb recipe, updated documentation, gave quiet default #64

Merged
merged 6 commits into from
Jun 15, 2016

Conversation

mhedgpeth
Copy link
Contributor

Description

The interval.rb recipe had a runtime error that made it clear that it wasn't being used. After futher inspection, it became clear that the default.rb recipe was ahead of the interval.rb recipe, and therefore the two should be merged. So the interval logic is inside of the default.rb recipe in this PR, where, if it is not defined, the interval is 0 seconds.

I went through the README.md document and updated outdated information and updated the documentation to reflect this pull request as well.

I also made a small change to make quiet mode default to true. The quiet mode functionality is broken in my mind, so I will create an issue for that and possibly another PR, since the scope of that would be rather large.

Issues Resolved

[List any existing issues this PR resolves]

Check List

I am still working to get the kitchen tests to work on my windows machine, so I haven't added testing. I wanted to submit the PR now because it is so large and I wanted to ensure that I am on the right track with these changes.

@mhedgpeth
Copy link
Contributor Author

There is still an issue with quiet mode, which is documented in #65; there will be another pull request for that after discussion occurs.

@@ -20,6 +20,14 @@
# These two attributes should only be set when connecting directly to Chef Compliance, otherwise they should be nil
token = node['audit']['token']
server = node['audit']['server']
interval_seconds = 0 # always run this by default, unless interval is defined
unless node['audit']['interval'].nil? && !node['audit']['interval']['enabled']
Copy link
Contributor

Choose a reason for hiding this comment

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

If I change line 24 to if then it becomes clearer and works as expected.

if !node['audit']['interval'].nil? && node['audit']['interval']['enabled']

@jeremymv2
Copy link
Contributor

@mhedgpeth Awesome!

This looks good given the comment above about changing line 24.

Also, could you include a unit test or two to verify the intended functionality?

If you can get those in I'll thumbs up.

@mhedgpeth
Copy link
Contributor Author

@jeremymv2 I fixed the if statement. I also added a unit test, but it's not passing at the moment and I don't know why. Got any hints on what I might be doing wrong?

@jeremymv2
Copy link
Contributor

jeremymv2 commented Jun 14, 2016

@mhedgpeth

This works:

  context 'when set to run on an interval and not due to run' do
    before(:each) do
      allow_any_instance_of(Chef::Resource).to receive(:profile_overdue_to_run?).and_return(false)
    end

    let(:chef_run) do
      runner = ChefSpec::ServerRunner.new(platform: 'centos', version: '6.5')
      runner.node.set['audit']['profiles'] = { 'admin/myprofile' => true }
      runner.node.set['audit']['interval']['enabled'] = true
      runner.converge(described_recipe)
    end

    it 'does not fetch or execute on compliance profile' do
      expect(chef_run).to_not fetch_compliance_profile('myprofile')
      expect(chef_run).to_not execute_compliance_profile('myprofile')
    end

    it 'converges successfully' do
      expect { chef_run }.to_not raise_error
    end
  end

I removed expect(chef_run).to_not execute_compliance_report('chef-server') because that is an outstanding "issue" that we need to address (we only want to execute the compliance_report resource IF the corresponding compliance_profile resource was executed - otherwise skip) Currently, the compliance_report resource executes every time, regardless.

@mhedgpeth
Copy link
Contributor Author

@jeremymv2 thanks for your help. I updated the unit test which provides testing to the feature. @chris-rock what needs to happen to merge this? I would like to use this as a basis for our implementation of compliance.

@chris-rock
Copy link
Contributor

Awesome work @mhedgpeth

@chris-rock chris-rock merged commit 7c9d1eb into chef-boneyard:master Jun 15, 2016
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.

3 participants