-
Notifications
You must be signed in to change notification settings - Fork 115
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
Conversation
Add ClientContext to InvokeRequest
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? |
There was a problem hiding this 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()} */ |
There was a problem hiding this comment.
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(() => { |
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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 } = {}) { |
There was a problem hiding this comment.
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)
Maybe I'm missing something but just running So I'm trying to understand what's this PR providing apart from that? |
The problem is that if I don't use:
...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 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. :) |
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 So, instead of
use
That way, serverless still packages the lambdas individually and correctly generates the package for the warmup plugin. |
@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 |
Hmmm, I was assuming that you have a single function pointing to your artifact. The real issue here is simply that when you have a top level 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 🙂 |
Thank you for your comment. Fully agree! I dont know if you noticied @juanjoDiaz but i really want to add this
|
Hi, serverless/serverless#5364 just got merge which essentially fix the packaging issues. Now (on next Serverless release) you can do:
So running serverless package with the above config will use Since serverless-plugin-warmup is set as Regarding the client context thingy, it seems like a pretty unrelated change to packaging. Could elaborate a bit on what's the use case? |
Awesome! True! I should do another PR regarding that! |
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
orc#