Skip to content

Custom ScheduleEvent rule description#257

Merged
DeviaVir merged 4 commits intomotdotla:masterfrom
EnriqueCanals:scheduled-event-rule-description
May 8, 2017
Merged

Custom ScheduleEvent rule description#257
DeviaVir merged 4 commits intomotdotla:masterfrom
EnriqueCanals:scheduled-event-rule-description

Conversation

@EnriqueCanals
Copy link
Contributor

This PR adds the ability to provide a custom description for a ScheduleEvent rule in the eventSourceFile. Although I find the default description of params.ScheduleName - params.ScheduleExpression to be sufficient in most cases, I think it would be useful to also have the ability to provide a custom description for these rules.

Copy link
Collaborator

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

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

good addition, but would like to see it slightly changed

"ScheduleState": "ENABLED",
"ScheduleExpression": "rate(1 hour)"
"ScheduleExpression": "rate(1 hour)",
"ScheduleDescription": "Run node-lambda-test-function once per hour"
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the default ${params.ScheduleName} - ${params.ScheduleExpression} is "enough" in 99% of the cases, it doesn't make sense to me to set this here and overwrite it for all functions.

Would it make more sense to not have this value here, but simply document it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense and I totally agree. Since there's no section dedicated to ScheduleEvents in the README, I think adding one to document some of the nuances with this feature is warranted. Anyway, if you agree I'd be more than happy to write up the documentation on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@EnriqueCanals that would be great! Thanks for taking the time to make this project better!

ScheduleEvents.prototype = {
_ruleDescription: (params) => {
return `${params.ScheduleName} - ${params.ScheduleExpression}`;
if (params.ScheduleDescription != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would also mean that this would have to check if ScheduleDescription is in params, and not null

Remove optional ScheduleEvents param from example event_sources.json
#### Optional Parameter
When using the eventSourceFile flag (-S or --eventSourceFile) to set a ScheduleEvent trigger, you can pass an optional _ScheduleDescription_ key into the ScheduleEvent object with a custom description for the CloudWatch event rule you are defining. By default, node-lambda generates a _ScheduleDescription_ for you based on the ScheduleName and ScheduleExpression of the rule.

#### Note on ScheduleState for ScheduleEvents
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeviaVir I added this subsection to the ScheduleEvents section because I feel this behavior deserves to be noted as it may be confusing to someone trying to deploy a Lambda with a disabled trigger. I wanted to bring it to your attention because I'm not certain if this is the intended behavior, but I can confirm that this is, in fact, how it currently works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not exactly how we like it, but long as it's been documented I think we're in a better state than before already. Thanks for that!

Copy link
Collaborator

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

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

Nice addition!

@DeviaVir DeviaVir merged commit 4ba9efa into motdotla:master May 8, 2017
@EnriqueCanals EnriqueCanals deleted the scheduled-event-rule-description branch May 15, 2017 21:52
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.

2 participants