Update NodeBuilder to Support Deployment of Nodejs Projects#30
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the NodeBuilder to support deployment of Node.js projects, bringing it to feature parity with the PythonBuilder. The key improvements include adding environment variable conversion from .env files to Azure App Settings, refactoring common deployment logic into a shared BuilderHelper class, and enhancing the Oryx manifest generation to support build commands.
- Refactored duplicate
ExecuteWithOutputAsyncandConvertEnvToAzureAppSettingslogic from platform-specific builders into a new sharedBuilderHelperclass - Extended the
IPlatformBuilderinterface to includeConvertEnvToAzureAppSettingsAsyncfor all platform builders - Enhanced Node.js deployment with
.deploymentfile creation and improved build command handling in the Oryx manifest - Updated deployment workflow to call environment variable conversion for all platforms, not just Python
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/BuilderHelper.cs | New helper class containing shared deployment logic for environment variable conversion and command execution |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/IPlatformBuilder.cs | Added ConvertEnvToAzureAppSettingsAsync method to interface for consistent environment variable handling |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/NodeBuilder.cs | Refactored to use BuilderHelper, added ConvertEnvToAzureAppSettingsAsync implementation, and improved build artifact deployment |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/PythonBuilder.cs | Refactored to use BuilderHelper for common deployment operations |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/DotNetBuilder.cs | Refactored to use BuilderHelper and added ConvertEnvToAzureAppSettingsAsync stub (returns true as .NET doesn't need it) |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/DeploymentService.cs | Updated deployment workflow to call ConvertEnvToAzureAppSettingsAsync for all platforms, not just Python |
| src/Microsoft.Agents.A365.DevTools.Cli/Models/OryxManifest.cs | Added BuildCommand property to support platform-specific build commands in manifest generation |
rahuldevikar761
requested changes
Nov 20, 2025
rahuldevikar761
left a comment
rahuldevikar761
left a comment
Collaborator
There was a problem hiding this comment.
Let's make sure to use toml or requirements and if we support both, then lets try to see see how we should resolve.
mengyimicro
previously approved these changes
Nov 21, 2025
mengyimicro
reviewed
Nov 21, 2025
rahuldevikar761
approved these changes
Nov 21, 2025
sellakumaran
pushed a commit
that referenced
this pull request
Feb 27, 2026
* Copy correct files into publish path + set up oryx manifest for nodejs * upload env variables for nodejs with refactor of common builder code * copilot suggestions * fix indentation --------- Co-authored-by: Jesus Terrazas <jterrazas@microsoft.com>
sellakumaran
added a commit
that referenced
this pull request
Jun 15, 2026
… GET stub in test - Collapse multi-line XML <summary>/inline comment blocks to a one-line why plus issue ref (pr-code-reviewer #30). - Stub the initial GET appRoleAssignments call explicitly in the Graph-and-az-rest-both-fail test so it no longer relies on BuildMockExecutor's default.
sellakumaran
added a commit
that referenced
this pull request
Jun 15, 2026
…terminism rule - pr-code-reviewer #30: mandatory-emit so the comment-density sweep is reported every run, not dropped as low-severity noise. - pr-code-reviewer #32: flag a test that relies on a shared mock-builder default instead of stubbing the call explicitly. - review-pr SKILL: read the working-tree copy of the rule files so a PR that adds a review rule is checked by that rule in the same run.
sellakumaran
added a commit
that referenced
this pull request
Jun 15, 2026
…ted (#461) * Fix #460: skip redundant agent-identity S2S grant when role is inherited When inheritable permissions are configured (allAllowed) and the blueprint SP holds the app roles, the agent identity inherits them automatically — the same basis the OBO/delegated path already relies on. The direct grant on the agent identity SP was therefore redundant and, when the CLI token cannot write app roles, produced a spurious "Action Required" PowerShell block on an otherwise successful Global Admin run. - Skip the per-identity grant (and the PowerShell prompt) when the blueprint grant and inheritance both succeeded; report the inherited grant as Granted. - When a direct grant is still needed (developer / non-inherited path), retry via az rest before falling back to PowerShell, matching the blueprint grant. - Correct stale README/CHANGELOG claim that Phase 2a/2b are always skipped for non-DW agents (untrue since #421). - Add review-time checks for comment-essay and CHANGELOG crispness (pr-code-reviewer #30/#31, review-pr SKILL) and codify both in CLAUDE.md and copilot-instructions. Verified live: setup completes cleanly with no PowerShell block, and query-entra reports the Observability app role granted on the blueprint SP with effective inheritance OK. * Address review comments on #461: crisp doc-comments; explicit az rest GET stub in test - Collapse multi-line XML <summary>/inline comment blocks to a one-line why plus issue ref (pr-code-reviewer #30). - Stub the initial GET appRoleAssignments call explicitly in the Graph-and-az-rest-both-fail test so it no longer relies on BuildMockExecutor's default. * Strengthen PR review tooling: mandatory comment-density emit, test-determinism rule - pr-code-reviewer #30: mandatory-emit so the comment-density sweep is reported every run, not dropped as low-severity noise. - pr-code-reviewer #32: flag a test that relies on a shared mock-builder default instead of stubbing the call explicitly. - review-pr SKILL: read the working-tree copy of the rule files so a PR that adds a review rule is checked by that rule in the same run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.