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

Allow soft warning when a hard error isn't required. #499

Closed
wants to merge 1 commit into from

Conversation

ncb000gt
Copy link
Member

GH-467 GH-478 GH-469 GH-470 GH-497 GH-498

Signed-off-by: Nick Campbell [email protected]

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #499 into master will decrease coverage by 0.73%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #499      +/-   ##
==========================================
- Coverage   92.35%   91.62%   -0.74%     
==========================================
  Files           3        3              
  Lines         353      358       +5     
  Branches      102      104       +2     
==========================================
+ Hits          326      328       +2     
- Misses         26       28       +2     
- Partials        1        2       +1     
Impacted Files Coverage Δ
lib/time.js 92.00% <33.33%> (-1.22%) ⬇️
lib/job.js 91.07% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a51726...e928c8c. Read the comment docs.

@ncb000gt ncb000gt force-pushed the soft-timeout branch 2 times, most recently from 7a6f93b to b79391b Compare May 28, 2020 03:00
… hard error isn't required.

Signed-off-by: Nick Campbell <[email protected]>
@utick
Copy link

utick commented Jul 15, 2020

any update?

@Bilonik
Copy link

Bilonik commented Aug 24, 2020

Update?

@ncb000gt
Copy link
Member Author

My hesitation with this PR is that it effectively reverts the change that was made to prevent infinite loops inside the cron tool. Of course this only happens in certain cases, and isn't all the time, but I want to support a better option. I'm thinking of making a change to this that resets the time by which the tool runs. I'll post here when I've settled on the best way to add this. Once I have a solution for this I'll push the code.

@aphix
Copy link

aphix commented Sep 3, 2020

@ncb000gt FYI it looks like this PR will always make softTimeout === true because of the or pipes (||) here: e928c8c (#499)

In line 58 of lib/job.js in the PR: softTimeout = cronTime.softTimeout || true

If provided initially via an object, the softTimeout option {softTimeout: false} would always be coerced to true based on the above logic, which would then be passed into the CronTime constructor.

@ncb000gt
Copy link
Member Author

🎉 This issue has been resolved in version 2.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@intcreator
Copy link
Collaborator

if anyone is curious on the decision making behind closing this PR check out this comment on #686

@ncb000gt
Copy link
Member Author

🎉 This issue has been resolved in version 3.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

6 participants