Skip to content

Conversation

ArthurMa1978
Copy link
Member

@ArthurMa1978 ArthurMa1978 commented Sep 12, 2025

What does this PR do?

This PR addresses part of the issue tracked in #226 by updating the SQL tool to eliminate its dependency on Azure.ResourceManager.ContainerService.
As a result, it reduces the MCP agent size by approximately 1.27 MB.

GitHub issue number?

#226

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Updated servers/Azure.Mcp.Server/CHANGELOG.md and/or servers/Fabric.Mcp.Server/CHANGELOG.md for product changes (features, bug fixes, UI/UX, updated dependencies)
  • For MCP tool changes:
    • One tool per PR: This PR adds or modifies only one MCP tool for faster review cycles
    • Updated servers/Azure.Mcp.Server/README.md and/or servers/Fabric.Mcp.Server/README.md documentation
    • Updated command list in /docs/azmcp-commands.md and/or /docs/fabric-commands.md
    • For new or modified tool descriptions, ran ToolDescriptionEvaluator and obtained a score of 0.4 or more and a top 3 ranking for all related test prompts
  • Extra steps for Azure MCP Server tool changes:
    • Updated test prompts in /docs/e2eTestPrompts.md
    • 👉 For Community (non-Microsoft team member) PRs:
      • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
      • Manual tests run: added comment /azp run mcp - pullrequest - live to run Live Test Pipeline

Copy link
Contributor

@Copilot 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 updates the AKS tool to eliminate its dependency on Azure.ResourceManager.ContainerService by migrating to Azure Resource Graph queries. This change reduces the MCP agent size by approximately 1.27 MB while maintaining the same functionality.

  • Replaced direct ARM API calls with Azure Resource Graph queries for better performance
  • Removed Azure.ResourceManager.ContainerService dependency to reduce package size
  • Added new model classes to handle JSON deserialization from Resource Graph responses

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tools/Azure.Mcp.Tools.Aks/src/Azure.Mcp.Tools.Aks.csproj Removed Azure.ResourceManager.ContainerService package dependency
tools/Azure.Mcp.Tools.Aks/src/Services/AksService.cs Migrated from ARM API calls to Resource Graph queries using BaseAzureResourceService
tools/Azure.Mcp.Tools.Aks/src/Services/Models/*.cs Added new model classes for JSON deserialization from Resource Graph results
tools/Azure.Mcp.Tools.Aks/src/Commands/*.cs Updated error handling to support KeyNotFoundException from Resource Graph queries
core/Azure.Mcp.Core/src/Services/Azure/BaseAzureResourceService.cs Made resourceGroup parameter nullable to support querying across all resource groups
servers/Azure.Mcp.Server/CHANGELOG.md Documented the refactoring changes

@@ -1,297 +0,0 @@
# PowerShell script to check for unused properties in Services/Models that are not used in Models mapping
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this script as it does not reliably detect unused resource properties in a generic manner.

@@ -23,8 +23,6 @@ internal sealed class SqlDatabaseProperties
public DateTimeOffset? CreatedOn { get; set; }
/// <summary> The current service level objective name of the database. </summary>
public string? CurrentServiceObjectiveName { get; set; }
/// <summary> The license type to apply for this database. `LicenseIncluded` if you need a license, or `BasePrice` if you have a license and are eligible for the Azure Hybrid Benefit. </summary>
public string? LicenseType { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by unused? It looks like an exported property, so not sure how you know it is unused by the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

The unused means not utilized by the MCP tool. I overlooked removing it in the previous PR when following up with your comment comment.

@feiskyer
Copy link
Contributor

Is there any consistency issue by switching to ARG? Per my understanding, there may be latencies there and hence some data may not be quired back immediately after new operations and hence may introduce confusion/troubles for scenarios like troubleshooting.

/hold

@ArthurMa1978
Copy link
Member Author

Is there any consistency issue by switching to ARG? Per my understanding, there may be latencies there and hence some data may not be quired back immediately after new operations and hence may introduce confusion/troubles for scenarios like troubleshooting.

/hold

Hey @feiskyer, what I know the consistency issue has been resolved and is no longer a concern. Latency remains a known issue, but it's not specific to ARG, so I don't believe it's a blocker.

charris-msft pushed a commit to charris-msft/mcp that referenced this pull request Sep 16, 2025
* Support starting server with multiple services

* Remove comments

* Fix and add test

* Fix minor issues in doc and style

* Reduce duplicated code

* Fix spell error

* Add a hint for starting server with multiple services

* Simplify the format command in in doc

* Use CommandLine library's Multiple Argument option to support passing multiple service values

* Use finer level Exception type
@joshfree joshfree moved this from Untriaged to In Progress in Azure MCP Server Sep 16, 2025
@feiskyer
Copy link
Contributor

Latency remains a known issue, but it's not specific to ARG, so I don't believe it's a blocker.

@ArthurMa1978 @joshfree Do we still want to implement something to detect the latency and refresh the data on such case? And does this change impact the write path (PUT/DELETE)? We're planning to add some create/write tools as well

@ArthurMa1978
Copy link
Member Author

Latency remains a known issue, but it's not specific to ARG, so I don't believe it's a blocker.

@ArthurMa1978 @joshfree Do we still want to implement something to detect the latency and refresh the data on such case? And does this change impact the write path (PUT/DELETE)? We're planning to add some create/write tools as well

The ARG provides sub-second query latency across subscriptions, though actual latency may vary depending on specific scenarios. This isn't an issue with ARG only, some mgmt. plane APIs may also not immediately reflect the latest data after changes are made. Therefore, I believe ARG is well-suited for any GET or LIST operations.
This update does not affect the write path. I'm currently working on a separate change to implement the write path using GenericResource, similar to this example: #456.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hallipr ok to remove?

@@ -23,8 +23,6 @@ internal sealed class SqlDatabaseProperties
public DateTimeOffset? CreatedOn { get; set; }
/// <summary> The current service level objective name of the database. </summary>
public string? CurrentServiceObjectiveName { get; set; }
/// <summary> The license type to apply for this database. `LicenseIncluded` if you need a license, or `BasePrice` if you have a license and are eligible for the Azure Hybrid Benefit. </summary>
public string? LicenseType { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by unused? It looks like an exported property, so not sure how you know it is unused by the caller.

@feiskyer
Copy link
Contributor

The ARG provides sub-second query latency across subscriptions, though actual latency may vary depending on specific scenarios.

This is a real issue that may cause AI Agent to perform wrong actions. If ARG is not required, I'd prefer we still keep the current logic to ensure data correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants