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

Always use a dedicated artifact for warmup function #69

Closed
wants to merge 2 commits into from

Conversation

richarddd
Copy link
Contributor

@richarddd richarddd commented Oct 5, 2018

Added zip functionality so that this plugin will always have a dedicated artifact.

Fixes: #25

This also solves issues when using a single artifact for all other functions when using for example java or c#

Add ClientContext to InvokeRequest
@marcfielding1
Copy link

marcfielding1 commented Oct 6, 2018

I'm not sure how active this repo is, I'm waiting for feedback on #24 for nearly two months - I was actually either going to fork this repo or create a new plugin this weekend if I hadn't heard back, @goncaloneves @RSchweizer is this repo still being maintained?

Copy link
Owner

@juanjoDiaz juanjoDiaz left a comment

Choose a reason for hiding this comment

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

Can you clarify the need to have your own packager instead just letting serverless do the packaging as it does with all other functions?

Haven't dig much into this but it sounds a bit like fighting the framework instead of using it :)

src/index.js Outdated
callback();
});
}`
/** Generated by Serverless WarmUP Plugin at ${new Date().toISOString()} */
Copy link
Owner

Choose a reason for hiding this comment

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

This produces incorrectly indented code.
I wish Node.js have a better way to do multiline text (like Scala for example) but, until it does, it was correct before, even if it isn't the prettiest...

src/index.js Outdated

/** Write warm up file */
return fs.outputFileAsync(this.pathFile, warmUpFunction)
return fs.outputFileAsync(this.pathFile, warmUpFunction).then(res => {
return this.createArtifact().then(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

return fs.outputFileAsync(this.pathFile, warmUpFunction)
  .then(res => this.createArtifact().then(() => red))

does exactly the same and is less verbose.

src/index.js Show resolved Hide resolved
@@ -49,7 +66,7 @@ class WarmUP {
* */
afterPackageInitialize () {
// See https://github.com/serverless/serverless/issues/2631
this.options.stage = this.options.stage
this.options.stage = this.options.stage
Copy link
Owner

Choose a reason for hiding this comment

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

Nice that you fix this style issues!

src/index.js Outdated
@@ -39,6 +43,19 @@ class WarmUP {
}
}

archiveDirectory (src, dest, format = 'zip', { archiverOptions = {}, directoryDestPath = false } = {}) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why having some normal properties and then some properties destructure from an object?

I'd flatten it completely archiveDirectory (src, dest, format = 'zip', archiverOptions = {}, directoryDestPath = false)

@juanjoDiaz
Copy link
Owner

Maybe I'm missing something but just running serverless package in any of my projects I get a zip file containing all my lambdas (including _warmup) and I also get a zip called warmUpPlugin containing this plugin.

So I'm trying to understand what's this PR providing apart from that?

@richarddd
Copy link
Contributor Author

richarddd commented Oct 7, 2018

The problem is that if I don't use:

package:
  individually: true

...and use a for example java, the warm-up folder must be moved inside my src folder. The problem is that this becomes a "catch 22" issue. I must build my shaddow jar before I run sls package, but the warmup folder must be moved to my resources be fore packing my jar or it would not be visible when running the warm-up lambda.

Further: It's nicer to have a few bytes of zip containing only this lambda than to have all other lambdas and dependencies in the warm-up lambda as well. :)

@juanjoDiaz
Copy link
Owner

Thanks, @richarddd! That clarifies the use case.

As I have commented in #25, would it be a solution to this problem to simply apply the package: artifact: at the function level instead of the service level?

So, instead of

package:
  artifacts: path/to/my-artifact.zip

functions:
  myFunction:
    ...

use

functions:
  myFunction:
    ...
    package:
      artifacts: path/to/my-artifact.zip

That way, serverless still packages the lambdas individually and correctly generates the package for the warmup plugin.

@richarddd
Copy link
Contributor Author

@juanjoDiaz I have to try this. Does the framework just upload one artifact for all (other) functions? Or is it copied? I do not wish to have multiple artifacts for all other lambdas except for this plugin

@juanjoDiaz
Copy link
Owner

Hmmm, I was assuming that you have a single function pointing to your artifact.
If you have multiple functions pointing to the same artifact I guess that it might get verbose to repeat it.

The real issue here is simply that when you have a top level package: artifact, Serverless doesn't package individual functions even if they are set as individually: true. So it's not really a problem of this plugin but a problem in Serverless package mechanism.

I just did a PR to Serverless fixing that, let's hope that it gets merged.

I think that your workaround is good. But I'd much rather fix the root cause than just work around it 🙂

@richarddd
Copy link
Contributor Author

Thank you for your comment. Fully agree!

I dont know if you noticied @juanjoDiaz but i really want to add this ClientContext to params as well:

const params = {
      // base64 encoded: {"custom":{"warmupEvent":"1"}}
      ClientContext: "eyJjdXN0b20iOnsid2FybXVwRXZlbnQiOiIxIn19",

@juanjoDiaz
Copy link
Owner

Hi,

serverless/serverless#5364 just got merge which essentially fix the packaging issues.

Now (on next Serverless release) you can do:

service: test

provider:
  name: aws
  runtime: nodejs8.10

package:
  artifact: my-app.zip

functions:
  normalFunc:
    handler: func/handler.func
    events:
      - http:
          path: func
          method: post
  individualFunc:
    handler: func/handler.func
    events:
      - http:
          path: func2
          method: post
    package:
       individually: true

So running serverless package with the above config will use my-app.zip as artifact for all functions but will create a zip for the ones set as package: individually: true like func/handler.func (the func folder needs to exist of course).

Since serverless-plugin-warmup is set as package: individually: true, it will always get its own zip.
So no need for custom zipping.

Regarding the client context thingy, it seems like a pretty unrelated change to packaging. Could elaborate a bit on what's the use case?

@richarddd
Copy link
Contributor Author

Awesome!

True! I should do another PR regarding that!

@richarddd richarddd closed this Oct 15, 2018
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.

None yet

4 participants