Better mixed runtime & function deploy handling #180
Conversation
|
@dschep Tried it out and here's what I get ( custom:
pythonRequirements:
dockerizePip: 'non-linux'
package:
individually: true
excludeDevDependencies: false
functions:
historical-validation:
handler: historical-validation.handler (also tried functions/historical-validation/historical-validation.handler)
runtime: python3.6
description: Historical validator
timeout: 300
module: functions/historical-validationDeployment: |
|
It does work, however, when I set |
|
yup. good catch. i'm able to reproduce the issue. |
|
Deploying node.js functions now works as well when |
|
Checked this against three of my codebases with a variety of configs, worked fine, passed all my codebases' CI-related tests. Glanced at the code also, seems good. Much appreciated. Before merging I would remove that erroneous console.log of artifacts. Of note: None of mine had the error like yours above with or without individually packaging. Might be a difference in the version of NodeJS (I'm using 6.13), but it should be pretty easy to fix (checking typeof, or so). |
|
Updated. Give it another shot @icj217. Glad to hear it works for you @AndrewFarley! |
|
Ahh @dschep warning do not merge... single function deploys is when that artifact problem occurs. I overlooked that in my brief tests since I haven't been able to do them in a while. Confirmed that problem occurs when using single function deploys, and confirmed that problem is gone now with your latest patch... but re-testing a bit now... and in my re-testing when deploying a single function I get major failures. The ZIP file we upload is now completely corrupt/invalid. It doesn't expand when re-downloaded from the Lambda console. So, something is super wrong with single function deploys. The package size looks alright, but something with the packager is corrupting the .zip (for me). On a side note: I'm so not used to waiting for requirements to compile any more with my caching MR i've been using exclusively everywhere. This testing took forever |
|
@dschep See pastebin if it helps: https://pastebin.com/cYPWDVKY |
|
Thanks for the warning @AndrewFarley. I'll have to really (as in actually test the deployed services) when i get time |
lib/inject.js
Outdated
| @@ -107,14 +107,14 @@ function injectAllRequirements() { | |||
| .map(func => | |||
| injectRequirements( | |||
| path.join('.serverless', func.module, 'requirements'), | |||
| func.package.artifact, | |||
| func.package ? func.package.artifact : artifact, | |||
There was a problem hiding this comment.
This did not fix the issue with doing a individually: true deployment. It complains about line 97.
There was a problem hiding this comment.
updated line 97 too.
|
Just tested out f4390b2 but it seems like when setting |
|
Now it's saying: |
|
oh yeah. duh. even the linter caught that. can't go reusing that same variable name. Updated again. |
|
@dschep Now its line 99. Seems like the |
|
Hmm. weird. I was able to deploy one of the test services before. but now when i try the following I am at getting the errors @AndrewFarley reported. |
|
Hmm.... @AndrewFarley |
|
@dschep Any update on my comment above? |
|
sorry. nope. haven't had time to prioritize on this since we don't use this feature ourselves. |
|
@AndrewFarley i merged #181 into this PR since i think it should address the corrupted zip issue |
|
@dschep Still got a problem when trying to deploy a single function with |
|
@dschep Seeing the same thing as @AndrewFarley |
|
@dschep Isn't this still the upstream problem of the artifact not being set properly...? Just tried upgrading serverless also, incase it was fixed, but nae. |
|
You are correct that package.artifact doesn't get set, but the packageFunction hook is passed the function object which should have an artifact set on it. https://github.com/UnitedIncome/serverless-python-requirements/pull/180/files#diff-168726dbe96b3ce427e7fedce31bb0bcR136 |
|
@dschep I fixed the problem with the following at the end of my inject.js file... this seems to fix all the problems. This inline if was in the |
|
Huh. Thought i'd done that for some reason. Added it. Thanks! 👍 |
|
Super excited to get single function deploys working again! :) I had a huge grin on my face just now seeing it work again, will (re)accelerate developer velocity over here. Cheers! |
lib/inject.js
Outdated
| @@ -117,7 +117,7 @@ function injectAllRequirements(funcArtifact) { | |||
| } else { | |||
| return injectRequirements( | |||
| path.join('.serverless', 'requirements'), | |||
| this.serverless.service.package.artifact, | |||
| this.serverless.service.package.artifact || funcArtifact | |||
There was a problem hiding this comment.
Getting a syntax error. I think you're missing the , at end of line.
There was a problem hiding this comment.
Ah. yes! (and that's probably why depcheck didn't think we use glob-all too)
|
Well, this confirmed works for me in all my serverless stacks, mixed or not, individually or not, and does single function deploys again. So I'm ready/eager for a merge, but can we get a few more eyes on it just incase? |
|
I'll go ahead and merge it (but not yet push to npm) and test it out on my personal projects tonight. Yeah, a known set of contributors/users who are willing to test features before release would be nice to have. I'll consider you as being on that list for now 😉 |
|
Hi guys, I was following the issue, especially after I updated the plugin before deploying an update of my Lambda function and it did not work! I ended up pulling the branch I forked to implement the per function requirements in order to be able to deploy, but now that I have a bot more time, I pulled master, and I'm afraid I still have the I'm gonna try to have a look at the code and I'll keep you updated. |
|
The problem seems to be that I have 2 functions defined in my project, and when I try to deploy the first one, the whole process (requirements installation, etc.) is done for both function. I don't think this is a recent issue, but now, the artifact property is not defined for the functions other than the one I want to deploy, hence the error. @AndrewFarley: you say it works for you, so I'm curious, do you have one function or multiple functions defined in your serverless.yml file? If you only have one, can you try adding another one and try again? If I'm correct, one solution would be to check what function is being processed. In almost all functions, we do something like: when it should be: Does it make sense? |
|
That makes sense @cgrimal! Feel free to make a PR if you have time before I do. |
|
I think I manage to have a "working" version, but the injection part was very long, and the vendors were not included. probably unrelated but not workable for me... This is the end of the day in my timezone and I'll be on annual leave tomorrow night, so I'm afraid I'll have to leave it to you guys, sorry. Here are the modifications I did in inject.js:
So it is very basic, I just iterate over the keys instead of the values, as this is the key being passed as CLI argument to deploy a single version, that I get from The same should be done in other modules. I hope it helps, really sorry to leave it like that. If there are still issues when I'm back in mid-May, I'll have a look! Many thanks |
|
No worries. Thanks for the notes. Enjoy your vacation! |
|
Re: @cgrimal Most of my stacks have multiple functions, and I injected one of a non-python runtime for some brief testing and didn't run into any issues, with or without individually set. I'm sure I didn't try out every single usage configuration path I'm sure though. Probably need more tests to help cover/catch all these things that seem to not get caught currently. Thanks for giving it another look |
|
I'm gonna try to add a second set of tests (
Anyother things i shoudl consider? (i'm even gonna also actually invoke the lambdas, which will prove that the packaged deps are really usable too!) |
|
@dschep Great plan, I think that mostly covers it... I agree don't run those in CI, but definitely make them something you/we can run locally to check/guarantee(ish) that things that are generated are sane. Definitely make sure to try out the lambdas, make sure (of course) when you try the lambdas, the code you run should try using an included super simple pip module as well, to catch incase it doesn't zip deps. That should be the same kind of test used for when doing a full deploy, or a single func and/or individually packaged deploy. |
fixes #161 and fixes #179
Could you test this out @icj217 & @AndrewFarley?