feature: change to zipdeploy & run from package #142
Conversation
mydiemho
commented
May 22, 2019
mydiemho
commented
- use the current Azure recommended zip deploy method to run function from deployment package
- remove support for individual function deploy since that's not supported by zipdeploy
src/services/functionAppService.ts
Outdated
| } | ||
| }; | ||
|
|
||
| await this._sendFile(requestOptions, functionZipFile); |
There was a problem hiding this comment.
Feels like we need some error handling/logging for when this fails. I'm also curious on timeout... if you have a big enough file, will it kill the upload? Or maybe all of this is covered in the _sendFile function?
There was a problem hiding this comment.
I was wondering too what’s the best way to handle this. _sendFile is an async function already but the zipdeploy api also let you set an isAsyncFlag (https://github.com/projectkudu/kudu/wiki/Deploying-from-a-zip-file-or-url#asynchronous-zip-deployment). Though I’m not sure how an async within an async would work.
There was a problem hiding this comment.
I think even a simple try/catch around the await that logs the error could be a start
|
Looking good. Does it make sense to add corresponding tests or are we going to revisit that for the deeper refactor efforts? |
|
@pjlittle we should discuss how we want to approach unit tests at standup |
tbarlow12
left a comment
tbarlow12
left a comment
There was a problem hiding this comment.
Looks great. +1 on unit tests
src/services/functionAppService.ts
Outdated
| } | ||
| }; | ||
|
|
||
| await this._sendFile(requestOptions, functionZipFile); |
There was a problem hiding this comment.
I think even a simple try/catch around the await that logs the error could be a start
1. remove individual function deploy support
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Added some feedback - Let's discuss if you have any questions.
src/services/functionAppService.ts
Outdated
| if (functionZipFile) { | ||
| this.serverless.cli.log(`-> Uploading ${functionZipFile}`); | ||
|
|
||
| const uploadUrl = `https://${functionAppName}${constants.scmDomain}${constants.scmZipDeployApiPath}`; |
There was a problem hiding this comment.
I don't think these constants should be used. In theory this should be the only function that should use this URL. Also - the scmDomain is not a constant and will change based on the configuration of the function app.
There was a problem hiding this comment.
scmDomain is coming from config.ts, how will that change with the function app?
| return response.data.value || []; | ||
| } | ||
|
|
||
| public async uploadFunctions(functionApp): Promise<any> { |
There was a problem hiding this comment.
I know we are using zipDeploy method for the service deployment - but I think we should keep a more generic name as the method name. Later on when we introduce runFromPackage we will have multiple branches within this method. Let's stick with uploadFunctions for now.
There was a problem hiding this comment.
just to be clear, I am adding runFromPackage in this PR already, it's the change in the arm template
| }, | ||
| { | ||
| "name": "WEBSITE_NODE_DEFAULT_VERSION", | ||
| "value": "8.11.1" |
There was a problem hiding this comment.
Just occurred to me that we should probably target the latest Node 8.x LTX, v8.16.0, which comes with NPM v.4.1: https://nodejs.org/en/download/releases/
I would assume this is supported?
There was a problem hiding this comment.
going to put this in a different pr and test :D
|
LGTM |
