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

feat: Move datadog hook back to before:package:createDeploymentArtifa… #404

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

astuyve
Copy link
Contributor

@astuyve astuyve commented Jun 29, 2023

…cts instead of after:package:Initialize

What does this PR do?

Had a great discussion w/ the author of serverless-plugin-warmup and realized we could probably fix the warmup issues with two changes:

  1. Moving our hook back in the logical order of hooks from after:package:initialize to before:package:createDeploymentArtifacts.

This is necessary to cooperate with the warmup plugin which needs to inject actual code after bundlers like webpack run.

Our library only sets config, so we only need to move the hook here to accommodate serverless-plugin-warmup.

Reasoning from author:

The warmup lambdas are added on the after:package:initialize event. Which is the same event used by the datadog plugin.
However, the webpack, bundle and similar plugins use the before:package:createDeploymentArtifacts event and break things.
For that, I need to reset the warmers during the before:package:createDeploymentArtifacts event.
I wish I could get rid of that hack but then I'd break the library for all people bundling their JS.
  1. For this to work, Datadog has to be listed after serverless-plugin-warmup. I've added this in the readme.

Motivation

Fully fixes our side of juanjoDiaz/serverless-plugin-warmup#231

Testing Guidelines

I ran this locally and tested to confirm it packages valid cloudformation for the warmer as well as my other function in the stack:
image

I manually added the ASTUYVE line into a local build of serverless-plugin-warmup to verify.

Additional Notes

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@astuyve astuyve requested review from a team as code owners June 29, 2023 20:01
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #404 (8b005ab) into master (415159b) will not change coverage.
The diff coverage is n/a.

❗ Current head 8b005ab differs from pull request most recent head 0d8770c. Consider uploading reports for the commit 0d8770c to get more accurate results

@@           Coverage Diff           @@
##           master     #404   +/-   ##
=======================================
  Coverage   75.43%   75.43%           
=======================================
  Files          11       11           
  Lines         871      871           
  Branches      271      271           
=======================================
  Hits          657      657           
  Misses        157      157           
  Partials       57       57           
Impacted Files Coverage Δ
src/index.ts 65.60% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@aliciascott aliciascott left a comment

Choose a reason for hiding this comment

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

A couple small changes in the readme

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
astuyve and others added 2 commits June 29, 2023 16:33
@astuyve astuyve requested a review from aliciascott June 29, 2023 20:34
Copy link
Contributor

@aliciascott aliciascott left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

@astuyve astuyve merged commit eb445a5 into master Jun 30, 2023
@astuyve astuyve deleted the aj/fix-warmup-plugin branch June 30, 2023 15:06
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.

4 participants