-
Notifications
You must be signed in to change notification settings - Fork 338
[app service] adding database connection #812
Conversation
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.
Pull Request Overview
This PR adds database connection functionality to the Azure App Service area, allowing users to configure database connections for web applications through the MCP tool. The implementation includes support for multiple database types (SQL Server, MySQL, PostgreSQL, CosmosDB) with automatic connection string generation.
Key changes:
- Implements App Service database connection commands and services
- Adds comprehensive test coverage for both unit and live testing scenarios
- Updates documentation with new command examples and parameters
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
docs/azmcp-commands.md | Documents new appservice database add command with examples and parameters |
areas/appservice/tests/test-resources.bicep | Defines test infrastructure for App Service and database resources |
areas/appservice/tests/AzureMcp.AppService.UnitTests/* | Unit tests for AppServiceService and DatabaseAddCommand |
areas/appservice/tests/AzureMcp.AppService.LiveTests/* | Live integration tests for database commands |
areas/appservice/src/AzureMcp.AppService/* | Core implementation including services, commands, models, and options |
Directory.Packages.props | Adds Xunit.SkippableFact package dependency |
.vscode/cspell.json | Adds spell check entries for appservice and sqlserver |
{ | ||
var options = BindOptions(parseResult); | ||
|
||
try |
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.
The CHANGELOG.md file needs to be updated to document this new database connection functionality. Add an entry describing the new appservice database add command in the appropriate release section.
Copilot uses AI. Check for mistakes.
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.
Agreed
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
areas/appservice/src/AzureMcp.AppService/Options/AppServiceOptionDefinitions.cs
Show resolved
Hide resolved
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
// SQL Server | ||
resource sqlServer 'Microsoft.Sql/servers@2023-05-01-preview' = { |
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.
We should consider if we use the sql from the /sql folder. I'm not sure if the deploy-testresources is setup to do dependencies, but I would like to not have to provision to sql servers and databases unless absolutely necessary.
docs/azmcp-commands.md
Outdated
- Service availability issues | ||
- Authentication errors | ||
- Service availability issues | ||
- Authentication errors |
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.
Also need to add to e2eprompts.md. Make sure you ask copilot to verify your PR against the checklist in new-command.md
022052a
to
be5215f
Compare
""" | ||
Add a database connection to an App Service. This command configures database connection | ||
settings for the specified App Service, allowing it to connect to a database server. | ||
"""; |
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.
Have you tried running the utility in eng\tools\ToolDescriptionConfidenceScore
to see if the description is good enough? You can find instructions on how to do that here :)
{ | ||
var options = BindOptions(parseResult); | ||
|
||
try |
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.
Agreed
return context.Response; | ||
} | ||
|
||
public record Result(DatabaseConnectionInfo ConnectionInfo); |
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.
We've been making the names of these records pretty verbose/descriptive. Should we use Result instead @jongio? After all, if we ever use them in other classes we could do so by using DatabaseAddCommand.Result
, for example.
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
areas/appservice/src/AzureMcp.AppService/Services/AppServiceService.cs
Outdated
Show resolved
Hide resolved
areas/appservice/tests/AzureMcp.AppService.LiveTests/Services/AppServiceServiceLiveTests.cs
Outdated
Show resolved
Hide resolved
areas/appservice/tests/AzureMcp.AppService.UnitTests/Services/AppServiceServiceTests.cs
Outdated
Show resolved
Hide resolved
.../appservice/tests/AzureMcp.AppService.UnitTests/Commands/Database/DatabaseAddCommandTests.cs
Outdated
Show resolved
Hide resolved
.../appservice/tests/AzureMcp.AppService.UnitTests/Commands/Database/DatabaseAddCommandTests.cs
Outdated
Show resolved
Hide resolved
|
||
// Assert | ||
Assert.NotNull(response); | ||
Assert.NotEqual(200, response.Status); |
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 think the response code on an error is 500
. We should probably compare against that. Not 200
could be 201
, which is still successful according to the REST spec.
…rvice.cs Co-authored-by: Copilot <[email protected]>
…rvice.cs Co-authored-by: Copilot <[email protected]>
b684b35
to
161e2cc
Compare
Sorry for the inconvenience, but we have moved the Azure MCP Server source code to https://github.com/microsoft/mcp. This change allows us to build any Microsoft MCP server with the same engineering system and allows us to brand any MCP server with either Azure or Microsoft branding. This repo change means that you need to move this PR to that new repo. Please do so and then add a link here to that new PR. We'll close this PR now and we look forward to seeing this over on the new repo. |
What does this PR do?
Adding database connection to webapp
[Any additional context, screenshots, or information that helps reviewers]
GitHub issue number?
[Link to the GitHub issue this PR addresses]
Pre-merge Checklist
CHANGELOG.md
for product changes (features, bug fixes, UI/UX, updated dependencies
).\eng\common\spelling\Invoke-Cspell.ps1
README.md
documentation/docs/azmcp-commands.md
/e2eTests/e2eTestPrompts.md
crypto mining, spam, data exfiltration, etc.
)/azp run azure - mcp
to run Live Test Pipeline