Skip to content

Fix events cron regex for schedule-expression param validation #12156

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 22, 2025
Merged

Fix events cron regex for schedule-expression param validation #12156

merged 1 commit into from
Jan 22, 2025

Conversation

anisaoshafi
Copy link
Contributor

@anisaoshafi anisaoshafi commented Jan 21, 2025 •
edited
Loading

Motivation

A user reported an inconsistent behaviour for events schedule-expression parameter validation between LS and AWS.
This caused the user to waste some time, since on LS the request succeeded, while it should have failed.

Previous result:

awslocal events put-rule \
--name my-scheduled-rule \
--schedule-expression 'cron(7 20 * * 1-7 *)'

{
"RuleArn": "arn:aws:events:us-east-1:000000000000:rule/my-scheduled-rule"
}

Expected result:

aws events put-rule \
--name my-scheduled-rule \
--schedule-expression 'cron(7 20 * * 1-7 *)'

An error occurred (ValidationException) when calling the PutRule operation: Parameter ScheduleExpression is not valid.

Changes

  • Improved the cron regex coverage.
  • Fixed a typo: CONNCTION_NAME_ARN_PATTERN

Testing

Existing test: tests_put_rule_with_schedule_cron successful ✅
New test: tests_put_rule_with_invalid_schedule_cron added.

@anisaoshafi anisaoshafi requested a review from bentsku January 21, 2025 16:55
@anisaoshafi anisaoshafi added aws:events Amazon EventBridge semver: patch Non-breaking changes which can be included in patch releases aws:scheduler Amazon EventBridge Scheduler and removed aws:events Amazon EventBridge labels Jan 21, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 21, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 10m 2s ⏱️ - 42m 46s
2 425 tests  - 1 555  2 321 ✅  - 1 343  104 💤  - 212  0 ❌ ±0 
2 427 runs   - 1 555  2 321 ✅  - 1 343  106 💤  - 212  0 ❌ ±0 

Results for commit 1971015. ± Comparison against base commit 2a9fdec.

This pull request removes 1566 and adds 11 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_invalid_schedule_cron[cron(0 dummy ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_invalid_schedule_cron[cron(7 20 * * NOT *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_invalid_schedule_cron[cron(71 8 1 * ? *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_invalid_schedule_cron[cron(INVALID)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(* * ? * SAT#3 *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 12 * * ? *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 2 ? * SAT *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 2 ? * SAT#3 *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/5 5 ? JAN 1-5 2022)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(15 10 ? * 6L 2002-2005)]
…

♻️ This comment has been updated with latest results.

@anisaoshafi anisaoshafi force-pushed the eventbridge-scheduler-fix branch 2 times, most recently from 2be7b8f to 6ef2376 Compare January 21, 2025 17:53
@anisaoshafi anisaoshafi force-pushed the eventbridge-scheduler-fix branch from 6ef2376 to 1971015 Compare January 21, 2025 17:54
joe4dev
joe4dev approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Thank you for the extended test coverage @anisaoshafi 💯
Thorough and clean ✨

@anisaoshafi anisaoshafi merged commit b4b0ab9 into master Jan 22, 2025
15 checks passed
@anisaoshafi anisaoshafi deleted the eventbridge-scheduler-fix branch January 22, 2025 10:39
@anisaoshafi anisaoshafi mentioned this pull request Jan 22, 2025
Merged
@anisaoshafi anisaoshafi added this to the 4.1 milestone Jan 23, 2025
@anisaoshafi anisaoshafi self-assigned this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@maxhoheiser maxhoheiser Awaiting requested review from maxhoheiser

@bentsku bentsku Awaiting requested review from bentsku

Assignees

@anisaoshafi anisaoshafi

Labels
aws:events Amazon EventBridge aws:scheduler Amazon EventBridge Scheduler semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@anisaoshafi @joe4dev