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

fix: add error message if unable to find cron expression for scheduled function #1123

Merged
merged 8 commits into from Jun 27, 2022

Conversation

jenae-janzen
Copy link
Contributor

πŸŽ‰ Thanks for submitting a pull request! πŸŽ‰

Summary

Fixes #<1108>

Checks to make sure that scheduled functions aren't missed. If the schedule helper is imported but not correctly invoked/exported, assumes user wanted to schedule the function and throws an error.

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’».
    This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
    a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)

@jenae-janzen jenae-janzen requested a review from danez June 23, 2022 21:24
@github-actions github-actions bot added the type: chore work needed to keep the product and development running smoothly label Jun 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 23, 2022

⏱ Benchmark results

Comparing with 7e21b63

largeDepsEsbuild: 7.7s

⬆️ 26.18% increase vs. 7e21b63

^                           8.4s                          
β”‚                           β”Œβ”€β”€β”                          
β”‚                           |  |                    7.7s  
β”‚                           |  |                    β”Œβ”€β”€β”  
β”‚                   6.9s    |  |                    |β–’β–’|  
β”‚ β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Όβ”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€6.5s────|β–’β–’|──
β”‚   5.7s    5.9s    |  |    |  |            β”Œβ”€β”€β”    |β–’β–’|  
β”‚   β”Œβ”€β”€β”    β”Œβ”€β”€β”    |  |    |  |    5.6s    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    β”Œβ”€β”€β”    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

largeDepsNft: 39.3s

⬆️ 23.63% increase vs. 7e21b63

^                          43.2s                          
β”‚                           β”Œβ”€β”€β”                          
β”‚                           |  |                   39.3s  
β”‚                           |  |                    β”Œβ”€β”€β”  
β”‚                           |  |                    |β–’β–’|  
β”‚                  32.7s    |  |                    |β–’β–’|  
β”‚ ──30sβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€β”Όβ”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€29.6s────|β–’β–’|──
β”‚   β”Œβ”€β”€β”   27.5s    |  |    |  |    27s     β”Œβ”€β”€β”    |β–’β–’|  
β”‚   |  |    β”Œβ”€β”€β”    |  |    |  |    β”Œβ”€β”€β”    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

largeDepsZisi: 55.1s

⬆️ 20.61% increase vs. 7e21b63

^                                                  55.1s  
β”‚                                                   β”Œβ”€β”€β”  
β”‚                  50.3s                            |β–’β–’|  
β”‚                   β”Œβ”€β”€β”                    46s     |β–’β–’|  
β”‚ ─43.7s───43.3sβ”€β”€β”€β”€β”Όβ”€β”€β”Όβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”Œβ”€β”€β”β”€β”€β”€β”€|β–’β–’|──
β”‚   β”Œβ”€β”€β”    β”Œβ”€β”€β”    |  |           41.7s    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            β”Œβ”€β”€β”    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |            |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    n/a     |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────────────┴──┴────┴──┴────┴──┴──>
    T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

Copy link
Contributor

@danez danez left a comment

Choose a reason for hiding this comment

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

I really like the simplicity of the approach. I thought there would be a lot more changes involved. Besides the one case that is missing (see comments), I think this is the way to go.

src/runtimes/node/in_source_config/index.ts Outdated Show resolved Hide resolved
src/runtimes/node/in_source_config/index.ts Outdated Show resolved Hide resolved
@jenae-janzen jenae-janzen requested a review from a team June 24, 2022 16:27
@jenae-janzen jenae-janzen marked this pull request as ready for review June 24, 2022 16:33
Copy link
Contributor

@danez danez left a comment

Choose a reason for hiding this comment

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

Hello from pto :) lgtm

If you want a release after merging you would need to change the type in the squash commit or pr title if using kodiak from chore to fix

@jenae-janzen jenae-janzen changed the title chore: add error message if unable to find cron expression for scheduled function fix: add error message if unable to find cron expression for scheduled function Jun 27, 2022
@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jun 27, 2022
@jenae-janzen jenae-janzen merged commit 2309f4b into main Jun 27, 2022
@jenae-janzen jenae-janzen deleted the chore/improve-scheduled-function-isc branch June 27, 2022 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants