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

Fix windows_task idle_time validation #6807

Merged
merged 1 commit into from Feb 7, 2018
Merged

Fix windows_task idle_time validation #6807

merged 1 commit into from Feb 7, 2018

Conversation

algaut
Copy link
Contributor

@algaut algaut commented Jan 29, 2018

Description

Two improvements on the idle_time validation function for the windows_task resource:

  • Compare frequency to :idle_time instead of comparing to an incomplete list of other frequencies.
  • Keep all the validation logic in the function instead of splitting it between the function and where the function is called.

Issues Resolved

I din't create an issue since it seemed like an easy fix, but the previous PR #6714 on the matter had some of my windows_task resources fail when using the :onstart frequency.

Check List

@algaut algaut requested a review from a team January 29, 2018 16:16
@algaut algaut closed this Jan 29, 2018
@algaut algaut reopened this Jan 29, 2018
@tas50
Copy link
Contributor

tas50 commented Jan 29, 2018

@algaut Thanks for taking the time to make sure this works in additional scenarios. Do you think you could take a peak at the current specs and add a spec or two for the scenarios you think this fixes. We're looking at doing some large scale refactoring of this codebase and without specs to handle each scenario there's a good chance we'll introduce a regression here in the future.

-Tim

@algaut
Copy link
Contributor Author

algaut commented Jan 30, 2018

Hi Tim,
Would some specs like those work for you? Not sure if I'm supposed to use simple logic during spec definitions, but this should cover most cases for what I changed

@tas50
Copy link
Contributor

tas50 commented Jan 31, 2018

@algaut I think those specs will certainly help us to avoid a regression here. Thanks. Any chance you can squash this down to a single commit and rebase it off master. We fixed some testing failures since you opened this up originally and that would let us get to green in appveyor testing.

@tas50
Copy link
Contributor

tas50 commented Feb 2, 2018

Fixes #6824

@algaut
Copy link
Contributor Author

algaut commented Feb 5, 2018

Should be good to go now @tas50

Two improvements:
- Compare frequency to :idle_time instead of comparing to an incomplete list of other frequencies
- Keep all the validation logic in the function instead of splitting it between the function and where the function is called

Added some specs tests too

Signed-off-by: Alan Gauthier <algaut35@gmail.com>
@tas50 tas50 merged commit a55fd7c into chef:master Feb 7, 2018
@tas50
Copy link
Contributor

tas50 commented Feb 7, 2018

Thanks @algaut

@lock
Copy link

lock bot commented Apr 14, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants