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

Migrate to chef_handler from resource-based audit #71

Closed
wants to merge 19 commits into from

Conversation

mhedgpeth
Copy link
Contributor

@mhedgpeth mhedgpeth commented Jun 29, 2016

Description

This is a major refactoring of the cookbook and is preliminary for feedback at this point. It addresses the problem with the current version defined as resources and thus reporting "converges" when doing reporting. Instead, this implements a chef_handler that does reporting, which fits the pattern much better.

Issues Resolved

#70

Check List

@mhedgpeth mhedgpeth changed the title Migrate to handler Migrate to chef_handler from resource-based audit Jun 29, 2016
@@ -1,78 +1,37 @@
---
driver:
name: dokken
name: vagrant
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a step backwards? Or was this just for your testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't run dokken on my windows machine. Plus I'm not convinced this ever worked, at least for the chef server. So it was for my testing. I'd be happy to roll it back, but don't see dokken as adding any benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually docker tests are faster / I think you can run it in travis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so faster tests + CI but no windows development support.I would understand if the former was deemed more valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to add multiple kitchen files as we are doing in https://github.com/chef/inspec
Reason: we cannot run vagrant in travis. Still like to see the vagrant option within the cookbook. Just move it to .kitchen.vagrant.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted back to dokken per @chris-rock and @bigbam505 suggestions. There is an alternative for vagrant called .kitchen.vagrant.yml

@vjeffrey
Copy link

ok, I pulled down this branch, resolved the conflicts, worked through a few more small code changes that needed to be done, and this seems to be working (tested against compliance server and visibility). That work is on this branch: https://github.com/chef-cookbooks/audit/compare/vj/try-to-merge-handler I can push those changes up here directly as well, I just pushed to a separate branch in case I, ya know, messed it all up :) If ppl could pls test that this is working the way we all expect it to...that would be awesome.

@jeremymv2
Copy link
Contributor

I propose we delay this until there is a better plan for fully replacing audit_mode with built-in inspec functionality in chef-client. There's just so much happening in this PR that I'm not sure it's worth the effort without very careful planning as to how inspec + chef-client work together.

@chris-rock
Copy link
Contributor

@mhedgpeth although we have not merged your PR, it was the inspiration to rewrite and clean up a lot in that cookbook. Again we thank you to demonstrate the use of Chef Handler and how that can help the audit cookbook. We mention this effort in our README via #130

We are looking forward to get more feedback and PRs!

@chris-rock chris-rock closed this Oct 27, 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.

6 participants