Skip to content

Fix slot deploy context value not being used #4617

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/commands/deploy/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { dotnetUtils } from '../../utils/dotnetUtils';
import { durableUtils } from '../../utils/durableUtils';
import { isPathEqual } from '../../utils/fs';
import { treeUtils } from '../../utils/treeUtils';
import { pickFunctionApp } from '../../utils/pickFunctionApp';
import { getWorkspaceSetting } from '../../vsCodeConfig/settings';
import { verifyInitForVSCode } from '../../vsCodeConfig/verifyInitForVSCode';
import { type ISetConnectionSettingContext } from '../appSettings/connectionSettings/ISetConnectionSettingContext';
Expand Down Expand Up @@ -54,7 +55,7 @@ export async function deploySlot(context: IActionContext, target?: vscode.Uri |
await deploy(context, target, functionAppId, new RegExp(ResolvedFunctionAppResource.pickSlotContextValue));
}

async function deploy(actionContext: IActionContext, arg1: vscode.Uri | string | SlotTreeItem | undefined, arg2: string | {} | undefined, _expectedContextValue?: string | RegExp): Promise<void> {
async function deploy(actionContext: IActionContext, arg1: vscode.Uri | string | SlotTreeItem | undefined, arg2: string | {} | undefined, expectedContextValue?: string | RegExp): Promise<void> {
const deployPaths: IDeployPaths = await getDeployFsPath(actionContext, arg1);

addLocalFuncTelemetry(actionContext, deployPaths.workspaceFolder.uri.fsPath);
Expand Down Expand Up @@ -96,9 +97,18 @@ async function deploy(actionContext: IActionContext, arg1: vscode.Uri | string |
}
}

const node: SlotTreeItem = await getDeployNode(context, ext.rgApi.tree, arg1, arg2, async () => {
return await getOrCreateFunctionApp(context)
});
let node: SlotTreeItem;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: Fixed slot deploy context value usage

The PR correctly addresses the deploy command logic by using expectedContextValue when it's provided and arg1 is undefined. This ensures that slot deploys properly filter to show only deployment slots rather than all function apps.

The addition of pickFunctionApp import and the conditional logic properly implements the intended behavior:

  • When deploying to slots (expectedContextValue is provided), use the filtered picker
  • Otherwise, use the existing deployment node logic

This fixes the issue where deploySlot command wasn't using its context filtering capability.

Review generated with Copilot

if (expectedContextValue && !arg1) {
// When deploying to a specific context (e.g., slot), use the picker with context filtering
node = await pickFunctionApp(context, {
expectedChildContextValue: expectedContextValue
});
} else {
// Use the regular deploy node logic for production deploys or when node is already provided
node = await getDeployNode(context, ext.rgApi.tree, arg1, arg2, async () => {
return await getOrCreateFunctionApp(context)
});
}

await node.initSite(context);
const site = node.site;
Expand Down
6 changes: 5 additions & 1 deletion src/templates/TemplateProviderBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,11 @@
}

public async getBackupTemplateVersion(): Promise<string> {
return (await AzExtFsExtra.readFile(await this.getBackupVersionPath())).toString().trim();
const versionContent = await AzExtFsExtra.readFile(await this.getBackupVersionPath());
if (!versionContent) {
throw new Error(localize('backupVersionFileEmpty', 'Backup template version file is empty or could not be read'));

Check failure on line 111 in src/templates/TemplateProviderBase.ts

View workflow job for this annotation

GitHub Actions / Build / Build

Unsafe call of an `any` typed value

Check failure on line 111 in src/templates/TemplateProviderBase.ts

View workflow job for this annotation

GitHub Actions / Build / Build

Unsafe argument of type `any` assigned to a parameter of type `string | undefined`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Missing import for localize function

The code uses localize('backupVersionFileEmpty', ...) but there's no import statement for the localize function in this file.

This will cause a compilation error. You need to add:

import { localize } from '../localize';

at the top of the file with the other imports.

Review generated with Copilot

}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: Fixed template version file error handling

This change properly addresses Issue #4599 where Cannot read properties of undefined (reading 'trim') error occurred when the backup template version file was empty or unreadable.

The fix adds appropriate null/undefined checking before calling .toString().trim(), providing a clear error message when the backup version file is problematic.

The error handling follows good practices:

  • Check for falsy values before proceeding
  • Throw a localized error message for better user experience
  • The error message is actionable (indicates the specific file is empty/unreadable)

Review generated with Copilot

return versionContent.toString().trim();
}

public async updateBackupTemplateVersion(version: string): Promise<void> {
Expand Down
Loading