Making changes to the publish CLI to for MOS Publish, Custom connector creation and App registrations#400
Making changes to the publish CLI to for MOS Publish, Custom connector creation and App registrations#400deepaligargms wants to merge 2 commits intomainfrom
Conversation
…r creation and App registrations
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
Updates the develop-mcp publish CLI flow to orchestrate Entra app creation and post-publish configuration (redirect URIs + PPMI scope grants), aligning it with the existing BYO registration orchestration pattern.
Changes:
- Extends
develop-mcp publishcommand options (adds--tenant-idand--service-tree-id) and wires execution through a newPublishCommandExecutor. - Expands publish request/response models to carry Entra app + connector-related fields needed for post-publish orchestration.
- Adjusts/realigns tests around publish command description and dry-run parsing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandTests.cs | Updates publish subcommand description assertion and validates new options exist. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopMcpCommandRegressionTests.cs | Refactors a publish integration test toward dry-run parsing behavior. |
| src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerResponse.cs | Adds publish response fields used for redirect-URI + PPMI permission back-fill. |
| src/Microsoft.Agents.A365.DevTools.Cli/Models/PublishMcpServerRequest.cs | Adds request fields for passing Entra app credentials/ids to the publish API. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommandExecutor.cs | New executor implementing publish orchestration and post-publish Graph configuration. |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopMcpCommand.cs | Updates publish subcommand wiring and adds new flags/options for orchestration inputs. |
| private async Task<EntraAppSet?> CreateEntraAppsAsync(ResolvedInput input, string tenantId, List<string> warnings) | ||
| { | ||
| var a365AppName = $"{input.ServerName}-A365Proxy"; | ||
| var publicClientsAppName = $"{input.ServerName}-PublicClients"; | ||
|
|
||
| _logger.LogDebug("Creating Entra application for A365 Proxy..."); | ||
| var a365App = await _graphApiService!.CreateEntraAppAsync(tenantId, a365AppName, serviceTreeId: input.ServiceTreeId); | ||
| if (a365App == null) | ||
| { | ||
| _logger.LogError("Failed to create Entra application '{AppName}'. Ensure you have Application.ReadWrite.All permission in the target tenant. Run with -v for details.", a365AppName); | ||
| return null; | ||
| } | ||
| _logger.LogInformation("Created Entra app '{AppName}' (clientId: {ClientId})", a365AppName, a365App.Value.ClientId); | ||
|
|
||
| var a365Secret = await _graphApiService.AddAppPasswordAsync(tenantId, a365App.Value.ObjectId); |
| ["--display-name", "-d"], | ||
| description: "Display name for the MCP server" | ||
| ); | ||
| description: "Display name for the MCP server (max 30 chars)"); |
| // Assert — description copy was extended in the BYO-parity work to call out the Entra app + | ||
| // back-fill orchestration the executor now performs, so match on the Azure-CLI-style verb prefix | ||
| // rather than the full string. | ||
| subcommand.Description.Should().StartWith("Publish an MCP server to a Dataverse environment"); | ||
|
|
| [Fact] | ||
| public async Task ServiceIntegration_PublishCommand_AcceptsAllNamedParameters() | ||
| { | ||
| // Core functionality test: Ensures publish command integration works correctly | ||
|
|
||
| // Verifies the publish CLI parses every documented flag without error. The publish flow now | ||
| // orchestrates Entra app creation + redirect-URI back-fill via GraphApiService (mirroring | ||
| // register-external-mcp-server), so end-to-end "params flow to PublishServerAsync" can't be | ||
| // exercised here without mocking Graph too — that path is covered by the | ||
| // <see cref="DryRunMode_NeverCallsActualServices"/> regression test and by manual E2E testing. | ||
|
|
||
| // Arrange | ||
| var testEnvId = "test-environment-123"; | ||
| var testServerName = "msdyn_TestServer"; | ||
| var testAlias = "test-alias"; | ||
| var testDisplayName = "Test Server Display Name"; | ||
|
|
||
| var mockResponse = new PublishMcpServerResponse | ||
| // Act — dry-run short-circuits the Graph + platform calls so this stays a pure CLI parsing test. | ||
| var result = await _command.InvokeAsync(new[] | ||
| { | ||
| Status = "Success", | ||
| Message = "Server published successfully" | ||
| }; | ||
|
|
||
| _mockToolingService.PublishServerAsync(testEnvId, testServerName, Arg.Any<PublishMcpServerRequest>()) | ||
| .Returns(mockResponse); | ||
|
|
||
| // Act | ||
| var result = await _command.InvokeAsync(new[] | ||
| { | ||
| "publish", | ||
| "publish", | ||
| "--environment-id", testEnvId, | ||
| "--server-name", testServerName, | ||
| "--alias", testAlias, | ||
| "--display-name", testDisplayName | ||
| "--display-name", testDisplayName, | ||
| "--dry-run", | ||
| }); | ||
|
|
||
| // Assert | ||
| // Assert — successful parse + dispatch, no service calls. | ||
| result.Should().Be(0); | ||
|
|
||
| await _mockToolingService.Received(1).PublishServerAsync( | ||
| testEnvId, | ||
| testServerName, | ||
| Arg.Is<PublishMcpServerRequest>(req => | ||
| req.Alias == testAlias && | ||
| req.DisplayName == testDisplayName) | ||
| ); | ||
| await _mockToolingService.DidNotReceive().PublishServerAsync(Arg.Any<string>(), Arg.Any<string>(), Arg.Any<PublishMcpServerRequest>()); | ||
| } |
| var testEnvId = "test-environment-123"; | ||
| var testServerName = "msdyn_TestServer"; | ||
| var testAlias = "test-alias"; | ||
| var testDisplayName = "Test Server Display Name"; | ||
|
|
||
| var mockResponse = new PublishMcpServerResponse | ||
| // Act — dry-run short-circuits the Graph + platform calls so this stays a pure CLI parsing test. | ||
| var result = await _command.InvokeAsync(new[] | ||
| { | ||
| Status = "Success", | ||
| Message = "Server published successfully" | ||
| }; | ||
|
|
||
| _mockToolingService.PublishServerAsync(testEnvId, testServerName, Arg.Any<PublishMcpServerRequest>()) | ||
| .Returns(mockResponse); | ||
|
|
||
| // Act | ||
| var result = await _command.InvokeAsync(new[] | ||
| { | ||
| "publish", | ||
| "publish", | ||
| "--environment-id", testEnvId, | ||
| "--server-name", testServerName, | ||
| "--alias", testAlias, | ||
| "--display-name", testDisplayName | ||
| "--display-name", testDisplayName, | ||
| "--dry-run", | ||
| }); |
| internal async Task ExecuteAsync(RawPublishArgs args, CancellationToken ct = default) | ||
| { | ||
| var input = ResolveInputs(args); | ||
| if (input is null) return; | ||
|
|
||
| DisplayPublishSummary(input); | ||
|
|
||
| if (input.DryRun) | ||
| { | ||
| _logger.LogInformation("[DRY RUN] Would create Entra apps '{A365}' and '{PublicClients}' in tenant", $"{input.Alias}-A365Proxy", $"{input.Alias}-PublicClients"); | ||
| _logger.LogInformation("[DRY RUN] Would call publish endpoint and back-fill redirect URI + PPMI scope on the created apps"); | ||
| return; | ||
| } | ||
|
|
|
|
||
| if (input.DryRun) | ||
| { | ||
| _logger.LogInformation("[DRY RUN] Would create Entra apps '{A365}' and '{PublicClients}' in tenant", $"{input.Alias}-A365Proxy", $"{input.Alias}-PublicClients"); |
| var request = new PublishMcpServerRequest | ||
| { | ||
| Alias = input.Alias, | ||
| DisplayName = input.DisplayName, | ||
| A365ProxyClientId = Guid.Parse(apps.A365AppClientId), |
| GraphApiService? graphApiService) | ||
| { | ||
| var command = new Command("publish", "Publish an MCP server to a Dataverse environment"); | ||
| var command = new Command("publish", "Publish an MCP server to a Dataverse environment. Creates the A365 Proxy + Public Clients Entra apps in your tenant, calls the platform publish endpoint, and back-fills redirect URIs and PPMI scope grants — same orchestration shape as register-external-mcp-server."); |
There was a problem hiding this comment.
Let's leave the description as is. No need to share these details to external users.
| ["--alias", "-a"], | ||
| description: "Alias for the MCP server" | ||
| ); | ||
| description: "Alias for the MCP server (used as the MCC row name and the CMS connector id)"); |
There was a problem hiding this comment.
Same. Internal details should not be shared with users
| ["--server-name", "-s"], | ||
| description: "MCP server name to publish" | ||
| ); | ||
| serverNameOption.IsRequired = false; // Allow null so we can prompt |
There was a problem hiding this comment.
Why changing this? This is false so that users can be prompted for this right?
| logger.LogError("Input validation failed: {Message}", ex.Message); | ||
| return; | ||
| } | ||
| var tenantIdOption = new Option<string?>( |
There was a problem hiding this comment.
I don't think this is needed
| Alias = alias, | ||
| DisplayName = displayName | ||
| }; | ||
| var serviceTreeIdOption = new Option<string?>( |
There was a problem hiding this comment.
This one is definitely not needed. Users won't have serviceTreeId
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public async Task<PublishMcpServerResponse?> PublishServerV2Async( |
There was a problem hiding this comment.
No need to do this on the CLI side. We can update just the existing command and its methods
| return tenantId; | ||
| } | ||
|
|
||
| private async Task<EntraAppSet?> CreateEntraAppsAsync(ResolvedInput input, string tenantId, List<string> warnings) |
There was a problem hiding this comment.
BYO has this exact logic no? Why not reuse?
| } | ||
| } | ||
|
|
||
| private async Task AddPpmiPermissionAsync( |
| PublicClientsAppName: publicClientsAppName); | ||
| } | ||
|
|
||
| private async Task ConfigureEntraAppsAsync( |
No description provided.