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

Adding an interval check, if you don't want to run every time #17

Merged
merged 2 commits into from Apr 27, 2016

Conversation

Projects
None yet
4 participants
@spuranam

spuranam commented Apr 1, 2016

No description provided.

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Apr 4, 2016

Collaborator

Thanks @spuranam for adding this PR. I like the addition. Could you do me one favor:

Collaborator

chris-rock commented Apr 4, 2016

Thanks @spuranam for adding this PR. I like the addition. Could you do me one favor:

@andy-dufour

This comment has been minimized.

Show comment
Hide comment
@andy-dufour

andy-dufour Apr 8, 2016

Contributor

thanks @chris-rock .. I think @spuranam accidently hit the new pull request button, I want to scope this delay interval per compliance profile and then I'll push some new changes up and ping for a review in the next couple of days ;)

Contributor

andy-dufour commented Apr 8, 2016

thanks @chris-rock .. I think @spuranam accidently hit the new pull request button, I want to scope this delay interval per compliance profile and then I'll push some new changes up and ping for a review in the next couple of days ;)

@andy-dufour

This comment has been minimized.

Show comment
Hide comment
@andy-dufour

andy-dufour Apr 13, 2016

Contributor

@chris-rock @spuranam @srenatus

This should be ready for review now. Let me know if you have any comments.

Contributor

andy-dufour commented Apr 13, 2016

@chris-rock @spuranam @srenatus

This should be ready for review now. Let me know if you have any comments.

@andy-dufour

This comment has been minimized.

Show comment
Hide comment
@andy-dufour

andy-dufour Apr 13, 2016

Contributor

Also - the reason this is important is if you have the audit cookbook in your nodes runlist.

There are customers that have inspec/compliance profiles that take 10+ minutes to run, I know one implemented thousands of lines of Inspec for DISA STIG on Windows that takes 13+ mins. This is a significant resource drain, and if your chef-client is executing frequently (say 30 mins or less) you're spending a lot of time auditing.

By making this audit interval say, once a day, a customer can restrict it from running all the time. By making this per profile we ensure that when a new profile is added it doesn't need to wait a maximum of interval to execute for the first time.

Contributor

andy-dufour commented Apr 13, 2016

Also - the reason this is important is if you have the audit cookbook in your nodes runlist.

There are customers that have inspec/compliance profiles that take 10+ minutes to run, I know one implemented thousands of lines of Inspec for DISA STIG on Windows that takes 13+ mins. This is a significant resource drain, and if your chef-client is executing frequently (say 30 mins or less) you're spending a lot of time auditing.

By making this audit interval say, once a day, a customer can restrict it from running all the time. By making this per profile we ensure that when a new profile is added it doesn't need to wait a maximum of interval to execute for the first time.

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Apr 16, 2016

Collaborator

@spuranam @andy-dufour This is a great addition. It starts to catch the use where the audit schedule is different from the converge run. Customers also mentioned requests like: "I want to run my audit every sunday morning". Therefore I am really happy to support the use case, but I am not sure about the interface yet. I think we need some time to learn more about it.

To enable the usecase quickly and give customers a way to use it, I would like to do the following:

Collaborator

chris-rock commented Apr 16, 2016

@spuranam @andy-dufour This is a great addition. It starts to catch the use where the audit schedule is different from the converge run. Customers also mentioned requests like: "I want to run my audit every sunday morning". Therefore I am really happy to support the use case, but I am not sure about the interface yet. I think we need some time to learn more about it.

To enable the usecase quickly and give customers a way to use it, I would like to do the following:

@srenatus

This comment has been minimized.

Show comment
Hide comment
@srenatus
Collaborator

srenatus commented Apr 18, 2016

@andy-dufour

This comment has been minimized.

Show comment
Hide comment
@andy-dufour

andy-dufour Apr 25, 2016

Contributor

@chris-rock how do you feel about this? (Once tests pass, of course)

Contributor

andy-dufour commented Apr 25, 2016

@chris-rock how do you feel about this? (Once tests pass, of course)

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Apr 25, 2016

Collaborator

@andy-dufour That looks great. One minor thing I was thinking about: Should we wrap the not_if around an include_recipe 'audit::default'? What do you think?

Collaborator

chris-rock commented Apr 25, 2016

@andy-dufour That looks great. One minor thing I was thinking about: Should we wrap the not_if around an include_recipe 'audit::default'? What do you think?

@andy-dufour

This comment has been minimized.

Show comment
Hide comment
@andy-dufour

andy-dufour Apr 25, 2016

Contributor

I like it for DRY reasons and it would be cleaner (plus if the default recipe changes we would pull in the upstream fixes).

The use case I'm trying to solve for and the reason I didn't do that is that I'm doing an interval per each individual compliance profile -- so if a user has a 'once per week' interval, and they add a profile in mid-week, they don't need to wait days for that profile to run the first time.

Perhaps we should simplify this though, and say it's a blanket 'all your profiles will run once per interval', until we flesh out the interface and bake it in a little more.

Contributor

andy-dufour commented Apr 25, 2016

I like it for DRY reasons and it would be cleaner (plus if the default recipe changes we would pull in the upstream fixes).

The use case I'm trying to solve for and the reason I didn't do that is that I'm doing an interval per each individual compliance profile -- so if a user has a 'once per week' interval, and they add a profile in mid-week, they don't need to wait days for that profile to run the first time.

Perhaps we should simplify this though, and say it's a blanket 'all your profiles will run once per interval', until we flesh out the interface and bake it in a little more.

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Apr 27, 2016

Collaborator

@andy-dufour sounds like a good approach. Lets try to harmonize this in future PRs.

I like to thank @spuranam and @andy-dufour for this great addition.

Collaborator

chris-rock commented Apr 27, 2016

@andy-dufour sounds like a good approach. Lets try to harmonize this in future PRs.

I like to thank @spuranam and @andy-dufour for this great addition.

@chris-rock chris-rock merged commit 1b19dfd into chef-cookbooks:master Apr 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment