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

address cron job confusion and update dependencies #54

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Conversation

brianberzins
Copy link
Collaborator

resolves #53 (thanks for bringing it to my attention @jnavarro86

There are a lot of details to this is #53, but the summary is that the cron library being used defaulted to a parser that used seconds (required) with day of week being optional. In newer versions of the library, the default parser still uses seconds, but includes better documentation and usability for making custom parse formats. I changed the parsing format to include optional seconds with required day-of-week, which is closer to the standard (differing only by inclusion of optional seconds).

This change should help alleviate any confusion.
* * * * * now complies with standard cron (starting with minutes)
* * * * * * starts with seconds and includes day of week

Before this change the behavior is actually:
* * * * * starts with seconds and does not include a day of week
* * * * * * starts with seconds and includes day of week

This is arguably a breaking change, or arguably just a bug fix. I'm going with the later given my own expectations of how it worked.

Along the way, I need to update one dependency, so I just updated a number. Credit to @jdharmon for doing the heavy lifting on dependency updates -- this should be compatible with the changes to #50

What's included:

  • update changelog (this had fallen slightly behind)
  • update golang version (which I ended up doing for local testing)
  • update the readme to document the new optional seconds on cron schedule
  • update vender dependencies (this includes the k8s dependencies that have been renamed)
  • use a custom parser to get optional seconds (for better compatibility and more accurate cron schedules)

What could "break"?

The case where things changes is when someone was using * * * * * . For anyone doing this, this PR will change that behavior to matching the standard cron format. If someone was intentionally relying on that being non-standard, then this might cause some minor pain in either the reaper running less frequently, or no longer being a valid cron format (which should error). I think this is reasonable to be considered as a fix rather than a breaking change.

Paging @slushpupie for a review 😄

@brianberzins
Copy link
Collaborator Author

Should mention that I got all paranoid about this and run a number of local tests (using example files) to make sure that everything was being run exactly when I expected it to.

- package: k8s.io/client-go
version: =v2.0.0
version: =v0.17.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I reading this right that we are going backwards in version number here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know it works, but that's a REALLY good question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the client-go versions, it looks like they switched mechanisms to match k8s versions, so 0.17.0 maps to 1.17.0. So it does appear this is a newer version, though not obviously so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't have a chance to look before you, but I was hoping it'd be something like that... Doing to cause a mess if k8s ever get to 2.0.0!

@brianberzins brianberzins merged commit 8b54fd1 into master Mar 27, 2020
@brianberzins brianberzins deleted the cron branch March 27, 2020 15:42
@brianberzins
Copy link
Collaborator Author

Thank you @slushpupie !

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

Successfully merging this pull request may close these issues.

Schedule doesn't seem to work correctly
2 participants