Skip to content

Cron does not correctly validate CloudWatch expressions where the minutes field contains a / #1

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

Closed
torybenya opened this issue Sep 27, 2022 · 6 comments
Assignees

Comments

@torybenya
Copy link

Hey folks - first of all, thank you for all the work writing the regex expressions. These seem/ed incredibly painful.

However as I was going through the test cases in my app for CloudWatch, I noticed there were 3 cases at the bottom of the table on this page (https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/ScheduledEvents.html) which were not properly validated:

  • 0/15 * * * ? * (Run every 15 minutes)
  • 0/10 * ? * MON-FRI * (Run every 10 minutes Monday through Friday)
  • 0/5 8-17 ? * MON-FRI * (Run every 5 minutes Monday through Friday between 8am and 5:55 pm UTC)

I believe the fix shouldn't be complicated, I have a locally modified version of the codebase running successfully locally just by adding an additional separate validator that is essentially just:
r"0\/{0-9}{1,2}

Is there any way this can be fixed? I would offer to open a PR, but unfortunately I may or may not be allowed to do so by the terms of my current employment. (I can look into this more if a PR would help, let me know.)

@grumBit
Copy link
Owner

grumBit commented Sep 29, 2022

Hi @torybenya, thank you for creating an issue for that error.

It turned out the regex validation for / was around the wrong way. It should have been checking that the increment came after the /.

The code and tests have been updated and release v1.0.6 has been uploaded to PyPI. Would you mind letting me know how it's working for you now please? If it's all good, we can close the issue.

Cheers!

@torybenya
Copy link
Author

torybenya commented Oct 3, 2022

@grumBit - thank you for the fast fix! I have one more question/request.
It appears that because of your project specs: https://github.com/grumBit/aws_cron_expression_validator/blob/master/pyproject.toml#L13 - this library is not compatible with our Python 3.8 environment. Is there any reason you specified 3.9 in particular? (Any new feature you're taking advantage of?) (Also, it's wild that our build process is interpreting 3.09 to mean 3.9 ... that might be us, not you!)
I didn't see anything that jumped out at me, and I'd be willing to test this with 3.8 - but I would need the specs updated, unfortunately, to do that (our build/deploy process won't let me override the specs in the package).

@grumBit
Copy link
Owner

grumBit commented Oct 4, 2022

@torybenya - I didn't have any technical reason for picking Python 3.9, it was just the one I'd been using when I created the package. I got the CI/CD to test using Python 3.7 (the oldest currently supported) and it worked fine, so I've made it the minimum dependency and released v1.0.7 of the package. I think the Python 3.09 version was just a typo, so I've dropped the 0.

Hopefully it'll now work happily with your environment. Let me know how you get on.

Cheers!

@grumBit grumBit self-assigned this Oct 4, 2022
@torybenya
Copy link
Author

@grumBit - thank you so much - everything is building happily now. Really appreciate your work on this.

@grumBit
Copy link
Owner

grumBit commented Oct 5, 2022

@torybenya - it's a pleasure! I'm glad people are getting use from my pet regex and PyPI packaging learning project.

I'm planning on adding some specific ValueError exceptions so that the error message string doesn't have to be inspected to determine what kind of error occurred. It'll all extend from ValueError, so will be completely backward compatible. I'm not sure if better error handling will be useful for what you're doing. If it is let me know and I'll get it done quickly.

Cheers!

@torybenya
Copy link
Author

@grumBit - thanks for the willingness to improve this module, but this works fine as-is for our purposes. Don't let that dissuade you though.

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

No branches or pull requests

2 participants