Fix rate.match is not a function issue #21
Conversation
|
Any update to when this will be merged? |
|
I'm also eager to have this fix merged, to work with newer Serverless! |
| @@ -70,7 +70,8 @@ function convertCrontabs() { | |||
| event.schedule.hasOwnProperty("timezone") | |||
| ) { | |||
| const schedule = event.schedule; | |||
| const match = schedule.rate.match(/^cron\((.*)\)$/); | |||
| const rate = Array.isArray(schedule.rate) ? schedule.rate[0] : schedule.rate; | |||
There was a problem hiding this comment.
I'm not familiar with this codebase but I'm afraid this approach might have a bug. If the rate is an array, I believe we should convert all entries in an array e.g. for config like this:
functions:
foo:
handler: foo.handler
events:
- schedule:
rate:
- cron(0 0/4 ? * MON-FRI *)
- cron(0 2 ? * SAT-SUN *)
The proposed approach would only address the first cron.
There was a problem hiding this comment.
You're right, the fix works fine with one schedule but breaks with multiple ones.
I wish I had time to propose a solution for the case with multiple schedules, but for now it's complicated... 😬
There was a problem hiding this comment.
multiples cron in the same rate is not a valid servelress configuration. This is just a plugin that resolves the timezone component, but the main configuration needs to follow the serverless configuration:
See for more details https://www.serverless.com/framework/docs/providers/aws/events/schedule
Multiple rate entries would work fine though:
functions:
crawl:
handler: crawl
events:
- schedule: cron(0 15 * * ? *)
- schedule: cron(0 12 * * ? *)
I understand this is not obvious to someone reading the proposed change, but it's important to understand this plugin is not working with latest versions of serverless and this change gives you a fix for it.
I would personally go ahead as-is and if required create another ticket to support multiple cron expressions (not needed IMO).
There was a problem hiding this comment.
Hey @ejoncas-rivalbet - sorry for not responding sooner, I've missed your comment.
multiples cron in the same rate is not a valid servelress configuration.
Why is that the case? I was testing it and it's working properly with syntax like this:
events:
- schedule:
rate:
- cron(0 0/4 ? * MON-FRI *)
- cron(0 2 ? * SAT-SUN *)
are you observing a different behavior? It's even listed as a valid approach in the docs that you've linked to: https://www.serverless.com/framework/docs/providers/aws/events/schedule#specify-multiple-schedule-expressions
As for going forward - given the fact that supporting multiple cron expressions is a small change in code, I think it should be done as a part of this PR.
|
Any update on this? We are unable to use this with the latest Serverless version. |
|
I'm taking over to finishing this and will also solve #13 as well |
|
New PR created for @types/serverless, please upvote so it can move along as well. |
|
#28 Is the new new hotness on the block to fix this issue. |
|
Fixed by #28 |
FIXES: #20
Solution by #20 (comment)