-
Notifications
You must be signed in to change notification settings - Fork 68
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
appservice: Refactor deployment code into steps #1554
Conversation
appservice/src/deploy/deploy.ts
Outdated
// Docker values point to the user's specific image, which we don't want to track | ||
return /^docker/i.test(linuxFxVersion) ? 'docker' : linuxFxVersion; | ||
const innerContext: InnerDeployContext = Object.assign(context, { site, fsPath, client, aspPromise }); | ||
const title: string = l10n.t('Deploying to function app "{0}"', site.fullName); |
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.
Function is used by App Service and Functions so title needs to either be generic or handle both cases.
const title: string = l10n.t('Deploying to function app "{0}"', site.fullName); | |
const title: string = l10n.t('Deploying to "{0}"', site.fullName); |
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.
I changed it to just "app".
callback: (zipStream: Readable) => Promise<AzExtPipelineResponse | void> | ||
}): Promise<AzExtPipelineResponse | void> { | ||
|
||
function onFileSize(size: number): void { | ||
context.telemetry.measurements.zipFileSize = size; | ||
const zipFileSize = vscode.l10n.t('Zip package size: {0}', prettybytes(size)); | ||
ext.outputChannel.appendLog(vscode.l10n.t('Zip package size: {0}', prettybytes(size)), { resourceName: site.fullName }); |
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.
ext.outputChannel.appendLog(vscode.l10n.t('Zip package size: {0}', prettybytes(size)), { resourceName: site.fullName }); | |
ext.outputChannel.appendLog(zipFileSize, { resourceName: site.fullName }); |
import { Progress, ProgressLocation, l10n, window } from "vscode"; | ||
import { InnerDeployContext } from "../IDeployContext"; | ||
|
||
export abstract class DeployExecuteBaseStep extends AzureWizardExecuteStep<InnerDeployContext> { |
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.
DeployExecuteStepBase
might be a better name.
Overall looks really good. I tested it myself with a few simple Function App deploys. |
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.
Fix da linter
Bump 👊 |
…-azuretools into nat/deploySteps
…-azuretools into nat/deploySteps
I think this is ready to review/approve btw. |
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.
I approve, however, lets properly plan out when we'll actually release this. I don't think we want to include this in the upcoming releases.
FYI, the zip deployment code has been modified on main. I don't know why there isn't any merge conflict. |
Oh never mind, I guess I still have |
No description provided.