Leverage new zipdeploy instead of manual zip upload #6
Conversation
| @@ -53,8 +53,7 @@ export async function zipDirectory(folderPath: string): Promise<string> { | |||
| zipper.pipe(zipOutput); | |||
| zipper.glob('**/*', { | |||
| cwd: folderPath, | |||
| dot: true, | |||
| ignore: 'node_modules{,/**}' | |||
| dot: true | |||
There was a problem hiding this comment.
Kudu requires you to zip up the node modules too?
There was a problem hiding this comment.
They don't require it, but the default behavior of zipdeploy assumes a ready-to-run app. See here for more info, specifically in the part about SCM_DO_BUILD_DURING_DEPLOYMENT.
We could go against the default behavior and set SCM_DO_BUILD_DURING_DEPLOYMENT to true, but I didn't think that was worth the added complexity. For example, do we overwrite the user's existing app setting if they have one? Do we put the app setting in a '.deployment' file? What if they already have a '.deployment' file?
Also - what if they don't have a standard nodejs project? Maybe they have a dotnet project with random files in a 'node_modules' folder. Maybe they have a nodejs project, but they install npm packages to a different folder.
I'm interested in what other people think, but see my PR description about how zip deploy now supports larger files. I didn't think it was as bad to include the modules directly.
There was a problem hiding this comment.
Discussed offline. I will keep this PR as-is and address at a later time. See issue #7
appservice/src/SiteWrapper.ts
Outdated
| @@ -70,8 +70,13 @@ export class SiteWrapper { | |||
| } | |||
|
|
|||
| public async deployZip(fsPath: string, client: WebSiteManagementClient, outputChannel: vscode.OutputChannel): Promise<void> { | |||
| const yes: string = 'Yes'; | |||
| const warning: string = `Are you sure you want to deploy to '${this.appName}'? This will overwrite any previous deployment and cannot be undone.`; | |||
There was a problem hiding this comment.
Should we add this prompt to App Services?
There was a problem hiding this comment.
App services calls this function, so no need to change that repo
| let deploymentId: string = 'latest'; | ||
| let deployment: DeployResult = await kuduClient.deployment.getResult(deploymentId); | ||
| while (!deployment.complete) { | ||
| if (!deployment.isTemp && deployment.id) { |
There was a problem hiding this comment.
Will DeployResult always conclude complete as true? What if there is no response or the client loses internet connection?
There was a problem hiding this comment.
Either it will complete as true or an error will be thrown. Either way we exit the loop
appservice/src/SiteWrapper.ts
Outdated
| @@ -70,8 +70,13 @@ export class SiteWrapper { | |||
| } | |||
|
|
|||
| public async deployZip(fsPath: string, client: WebSiteManagementClient, outputChannel: vscode.OutputChannel): Promise<void> { | |||
| const yes: string = 'Yes'; | |||
| const warning: string = `Are you sure you want to deploy to '${this.appName}'? This will overwrite any previous deployment and cannot be undone.`; | |||
There was a problem hiding this comment.
Double quote in the warning message.
This relies on PR #5 (the travis CI build will fail until that package is updated)
Fixes #4
Fixes microsoft/vscode-azureappservice#116
The updated zipdeploy now supports much larger files, so I decided to remove our logic that excluded 'node_moduels'. Zipdeploy has intelligent file copying, etc., so I figured just better to stick with their defaults. I was able to deploy this:
Resulting in a 1.4 GB site:
On B1 app service plan: