Skip to content

Conversation

@lambrianmsft
Copy link
Contributor

@lambrianmsft lambrianmsft commented Dec 3, 2025

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

This PR implements container support for the VS Code Logic Apps extension by removing local dependency management and adding containerized runtime support. The changes include:

Removal of binary validation and installation logic for .NET, Node.js, and Azure Functions Core Tools
Addition of Azure Functions Core Tools download in the container Dockerfile
Simplification of project setup by using system-installed binaries instead of managed dependencies
Updated VS Code tasks to use standard commands (dotnet, func) instead of configuration-based paths

Impact of Change

  • Users: Simplified setup experience when using containers - no more dependency management
  • Developers: Cleaner codebase with reduced complexity around binary management
  • System: Container-based workflow with pre-installed runtime dependencies

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: Container development environment

Contributors

@ccastrotrejo

Screenshots/Videos

Copilot AI review requested due to automatic review settings December 3, 2025 01:37
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: feat(vscode): Container support for extension dependencies
  • Issue: The title is generally good and follows conventional commit style (feat scope). There is a trailing space at the end of the title and you may want to slightly clarify whether this is for the extension or extension's dependency management (optional).
  • Recommendation: Remove the trailing space. Consider a slightly clearer variant if useful, for example: feat(vscode): add container-based support for extension dependencies or feat(vscode): containerized dependency support for VS Code extension.

Commit Type

  • Properly selected (feature).
  • Only one commit type is selected, which is correct and matches the changes described.

Risk Level

  • The PR body selects High risk and the PR has a Risk:High label. These match and are appropriate given the scale of the change (109 files changed, ~7.5k additions, ~3.3k deletions).

What & Why

  • Current: This section is present and describes the change: removal of local dependency management and adding containerized runtime support, updates to tasks and simplified setup.
  • Issue: None — content is clear and concise.
  • Recommendation: (Optional) Link to design doc or developer guidance for running locally vs in container if you have one.

Impact of Change

  • The section is present and identifies Users, Developers, and System impacts.
  • Recommendation: Minor formatting fix: add a space after the Users colon to match the other lines (currently it's - **Users**: <!-- User-facing changes, if any -->Simplified...). Example formatting:
    • Users: Simplified setup experience when using containers - no more dependency management
    • Developers: Cleaner codebase with reduced complexity around binary management
    • System: Container-based workflow with pre-installed runtime dependencies

Test Plan

  • Manual testing is marked as completed and the environment is specified (container development environment). This is OK for this change, but please note that for high-risk changes adding unit and/or E2E tests where feasible strengthens the PR and helps reviewers feel confident in merging.

Contributors

  • Contributors are listed (@ccastrotrejo). Good practice.

⚠️ Screenshots/Videos

  • No screenshots/videos provided. This is acceptable if there are no UI/visual changes. If this change touches UI or developer UX flows that would benefit from a short demo (screenshots/video/GIF), please add them.

Summary Table

Section Status Recommendation
Title Remove trailing space; optionally clarify wording
Commit Type No change needed
Risk Level Matches label; appropriate for scope
What & Why Consider linking design doc if available
Impact of Change Fix small formatting; expand if needed
Test Plan Consider adding unit/E2E tests if possible
Contributors No change needed
Screenshots/Videos ⚠️ Add if there are UI/UX changes

Summary: Your PR body follows the required template and is largely complete and accurate. The risk level you chose (High) is appropriate based on the scope and file changes noted. Please apply these minor edits before marking the PR as ready for final review:

  • Remove trailing space in the PR title and optionally make wording slightly clearer.
  • Fix the small formatting issue under Impact (add a space after the Users colon).
  • (Optional) Add links to any design/implementation doc and consider adding unit/E2E tests where feasible for higher confidence.
  • If any UI or UX visible changes exist, include screenshots or a short video.

Please update the PR title/body accordingly and re-submit when ready. Thank you for keeping the PR descriptive and well-structured!


Last updated: Fri, 12 Dec 2025 01:51:16 GMT

@lambrianmsft lambrianmsft added the risk:high High risk change requiring careful review label Dec 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the automatic binary dependency management system and introduces dev container support for Logic Apps extension dependencies. The extension now expects runtime dependencies (Node.js, .NET SDK, Azure Functions Core Tools) to be pre-installed in the environment (either locally or via containers) rather than attempting to manage them programmatically. The PR also consolidates to Azure Functions v4 only, removing support for older runtime versions.

Key changes:

  • Adds complete dev container configuration with multi-platform Docker image support (amd64/arm64)
  • Removes ~5000 lines of binary download/installation/validation code
  • Introduces getPublicUrl utility to map localhost URLs to external URLs for container environments
  • Hardcodes extension bundle version to 1.131.9 and removes dynamic version resolution
  • Updates API paths by adding missing leading slashes to managementApiPrefix usage

Reviewed changes

Copilot reviewed 91 out of 91 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
libs/vscode-extension/src/lib/services/httpClient.ts Fixed Authorization header to only include when needed (avoiding empty string)
libs/vscode-extension/src/lib/models/project.ts Removed unused OpenBehavior options
libs/vscode-extension/src/lib/models/host.ts Removed IHostJsonV1 interface (v1 no longer supported)
libs/vscode-extension/src/lib/models/functions.ts Removed FuncVersion v1-v3 (only v4 supported)
apps/vs-code-designer/src/constants.ts Added EXTENSION_BUNDLE_VERSION constant, removed dependency settings/paths
apps/vs-code-designer/src/main.ts Removed onboarding flow, simplified activation
apps/vs-code-designer/src/onboarding.ts Deleted entire onboarding module
apps/vs-code-designer/src/app/utils/extension.ts Added getPublicUrl utility for container URL mapping
apps/vs-code-designer/src/app/utils/binaries.ts Deleted entire binary management module (~441 lines)
apps/vs-code-designer/src/app/utils/bundleFeed.ts Removed dynamic bundle download, kept only path resolution
apps/vs-code-designer/src/app/utils/startRuntimeApi.ts Updated to use getPublicUrl for container support
apps/vs-code-designer/src/assets/container/* Added Dockerfile, devcontainer.json, build script, and documentation
apps/vs-code-designer/src/package.json Removed 50+ dependency-related settings and 3 commands
Multiple task/settings files Hardcoded tool commands ('dotnet', 'func') instead of config variables
Multiple test files Updated/removed tests for deleted functionality

Comment on lines +66 to +68
wget "${EXTENSION_BUNDLE_CDN_URL}/ExtensionBundles/Microsoft.Azure.Functions.ExtensionBundle.Workflows/${EXTENSION_BUNDLE_VERSION}/${EXTENSION_BUNDLE_FILENAME}" -O "/tmp/${EXTENSION_BUNDLE_FILENAME}"; \
mkdir -p "/${EXTENSION_BUNDLE_FOLDER_PATH}/Microsoft.Azure.Functions.ExtensionBundle.Workflows/${EXTENSION_BUNDLE_VERSION}"; \
unzip -q "/tmp/${EXTENSION_BUNDLE_FILENAME}" -d "/${EXTENSION_BUNDLE_FOLDER_PATH}/Microsoft.Azure.Functions.ExtensionBundle.Workflows/${EXTENSION_BUNDLE_VERSION}"; \
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The extension bundle is downloaded via wget from functionscdn.azureedge.net and unzipped without any checksum or signature verification. If the CDN or connection is compromised, a tampered bundle could be injected, leading to execution of malicious code during development. Add integrity checks (e.g., pinned SHA256 for the specific EXTENSION_BUNDLE_VERSION, or a signed manifest) and verify before unzipping:

wget "$EXTENSION_BUNDLE_CDN_URL/.../$EXTENSION_BUNDLE_FILENAME" -O "/tmp/$EXTENSION_BUNDLE_FILENAME"
echo "<expected-sha256>  /tmp/$EXTENSION_BUNDLE_FILENAME" | sha256sum -c -
unzip -q "/tmp/$EXTENSION_BUNDLE_FILENAME" -d "/$EXTENSION_BUNDLE_FOLDER_PATH/.../$EXTENSION_BUNDLE_VERSION"

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +83
wget "https://github.com/Azure/azure-functions-core-tools/releases/download/${FUNCTIONS_CORE_TOOLS_VERSION}/${FILENAME}" -O "/tmp/${FILENAME}"; \
mkdir -p "/${FUNCTIONS_CORE_TOOLS_FOLDER_PATH}"; \
unzip -q "/tmp/${FILENAME}" -d "/${FUNCTIONS_CORE_TOOLS_FOLDER_PATH}"; \
rm -f "/tmp/${FILENAME}"; \
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Azure Functions Core Tools are downloaded from GitHub releases via wget and extracted without verifying authenticity (no checksum/signature). This enables supply-chain attacks where a compromised release or MITM injects malicious binaries. Pin and verify checksums (e.g., SHA256 of Azure.Functions.Cli...zip) or use signed packages; verify before unzip:

wget "https://github.com/Azure/azure-functions-core-tools/releases/download/${FUNCTIONS_CORE_TOOLS_VERSION}/${FILENAME}" -O "/tmp/${FILENAME}"
echo "<expected-sha256>  /tmp/${FILENAME}" | sha256sum -c -
unzip -q "/tmp/${FILENAME}" -d "/${FUNCTIONS_CORE_TOOLS_FOLDER_PATH}"

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +53
curl -sSL https://dot.net/v1/dotnet-install.sh | bash /dev/stdin --channel 8.0 --install-dir /usr/share/dotnet; \
curl -sSL https://dot.net/v1/dotnet-install.sh | bash /dev/stdin --channel 6.0 --install-dir /usr/share/dotnet; \
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

curl is piped directly to bash for installing .NET (curl -sSL https://dot.net/v1/dotnet-install.sh | bash /dev/stdin ...) without any integrity verification. An attacker controlling the network or CDN could supply a malicious script leading to arbitrary code execution at build time. Download the script first and verify its checksum/signature (e.g., SHA256 pinned hash or GPG) before execution, or vendor the installer and verify it; example:

curl -sSL https://dot.net/v1/dotnet-install.sh -o /tmp/dotnet-install.sh
sha256sum -c /tmp/dotnet-install.sh.sha256  # or pin a known hash
bash /tmp/dotnet-install.sh --channel 8.0 --install-dir /usr/share/dotnet

Copilot uses AI. Check for mistakes.
@lambrianmsft lambrianmsft marked this pull request as draft December 3, 2025 01:46
@@ -0,0 +1,39 @@
{
"name": "LogicAppContain",
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

@@ -0,0 +1,39 @@
{
"name": "LogicAppContain",
"image": "carloscastrotrejo/logicapps-dev:latest",
Copy link
Contributor

Choose a reason for hiding this comment

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

Lests change the name

import { logExtensionSettings, logSubscriptions, runWithDurationTelemetry } from './app/utils/telemetry';
import { registerAzureUtilsExtensionVariables } from '@microsoft/vscode-azext-azureutils';
import { getAzExtResourceType, getAzureResourcesExtensionApi } from '@microsoft/vscode-azureresources-api';
// import { tryReopenInDevContainer } from './app/utils/devContainer';
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Comment


export function parseHostJson(data: unknown, version: FuncVersion | undefined): IParsedHostJson {
return version === FuncVersion.v1 ? new ParsedHostJsonV1(data) : new ParsedHostJsonV2(data);
export function parseHostJson(data: unknown): IParsedHostJson {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: lets just use ParsedHostJsonV2 directly

@hartra344 hartra344 added risk:low Low risk change with minimal impact and removed risk:high High risk change requiring careful review labels Dec 8, 2025
@hartra344 hartra344 added risk:high High risk change requiring careful review and removed risk:low Low risk change with minimal impact labels Dec 8, 2025
export const OpenBehavior = {
addToWorkspace: 'AddToWorkspace',
openInNewWindow: 'OpenInNewWindow',
openInCurrentWindow: 'OpenInCurrentWindow',
Copy link
Contributor

Choose a reason for hiding this comment

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

We are removing these behavior options entirely? Is there any other use case

export const FuncVersion = {
v1: '~1',
v2: '~2',
v3: '~3',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing older func versions?

"category": "Azure Logic Apps"
},
{
"command": "azureLogicAppsStandard.validateAndInstallBinaries",
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these still necessary commands for non-dev container projects?

"type": "string",
"description": "The path for Azure Logic Apps extension runtime dependencies."
},
"azureLogicAppsStandard.dotnetBinaryPath": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above for these settings

@@ -0,0 +1,433 @@
# CreateLogicAppProject.test.ts - Test Coverage Analysis
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to make these notes inline in the tests file? Or is this just a temporary file for debugging

@@ -0,0 +1,449 @@
# CreateLogicAppWorkspace.test.ts - Test Coverage Summary
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as other .md file, should this be inline?

import * as path from 'path';
import { ext } from '../../../../extensionVariables';

export class DevcontainerStep extends AzureWizardPromptStep<IProjectWizardContext> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this seems to function as an execute step rather than prompt step

import { getGlobalSetting } from '../../utils/vsCodeConfig/settings';
import type { IActionContext } from '@microsoft/vscode-azext-utils';

export async function installDotNet(context: IActionContext, majorVersion?: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this and related files are removed- aren't they still relevant for non dev containers?

// Designer
export const managementApiPrefix = '/runtime/webhooks/workflow/api/management';
export const designerStartApi = '/runtime/webhooks/workflow/api/management/operationGroups';
export const managementApiPrefix = 'runtime/webhooks/workflow/api/management';
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like some usages of managementApiPrefix still assume the '/' is included (unless getPublicUrl returns url with '/' at end)

export async function writeTasksJson(context: IWebviewProjectContext, vscodePath: string): Promise<void> {
const tasksJsonPath: string = path.join(vscodePath, tasksFileName);
const tasksJsonFile = 'TasksJsonFile';
const tasksJsonFile = context.isDevContainerProject ? 'DevContainerTasksJsonFile' : 'TasksJsonFile';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there support for working on a 'dev container project' without dev containers, i.e. using the existing solution as a fallback?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep lets remove this md files that are just temporary for the agent

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these mockups are already mocked up in the following file - apps/vs-code-designer/test-setup.ts
Lets move the mockups that are not specific to this file and can be used in all

@hartra344 hartra344 changed the title feat(vscode): Container support for extension dependencies feat(vscode): Container support for extension dependencies Dec 9, 2025
… to test adding a project to a devcontainer workspace and to verify the structure of the workspace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:high High risk change requiring careful review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants