-
Notifications
You must be signed in to change notification settings - Fork 227
Added update-metadata tool #12761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added update-metadata tool #12761
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 introduces a new MetadataUpdateTool for updating package metadata files in Azure SDK packages, along with supporting infrastructure including a ProcessConfigurationService, ResponseFactory, and language-specific package update implementations. The changes also refactor the configuration helper to support multiple configuration types beyond just build operations.
Key changes:
- Added
MetadataUpdateToolwith configurable script or language-specific update logic - Introduced
ProcessConfigurationServiceto centralize process execution logic - Extended
SpecGenSdkConfigHelperto support multiple configuration types (Build, UpdateChangelog, UpdateVersion, UpdateMetadata, UpdateCi) - Added language-specific package update service interface and stub implementations for all supported languages
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| SdkBuildTool.cs | Updated to use refactored configuration helper with new enum types |
| MetadataUpdateTool.cs | New tool implementing metadata update functionality with script-based or language-specific fallback logic |
| ServiceRegistrations.cs | Registered new services and language-specific package update implementations |
| ProcessConfigurationService.cs | New service centralizing process creation and execution with standardized response handling |
| LanguageService.cs | Enhanced with GetPackageInfo method and moved to Services namespace |
| IProcessConfigurationService.cs | Interface for the new process configuration service |
| ResponseFactory.cs | Factory for creating standardized success/failure responses |
| IResponseFactory.cs | Interface for the response factory |
| SpecGenSdkConfigHelper.cs | Refactored to support multiple configuration types and improved logging |
| PackageResponseBase.cs | Updated namespace import |
| Language-specific PackageUpdate classes | Stub implementations with NotImplementedException for future development |
| ILanguagePackageUpdate.cs | Interface defining package update operations |
Comments suppressed due to low confidence (1)
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs:1
- Using
await Task.CompletedTaskbefore throwing an exception is unnecessary and serves no purpose. The await statement should be removed, and the method can simply throw the NotImplementedException directly. This pattern appears in all language-specific PackageUpdate classes.
using Azure.Sdk.Tools.Cli.Models;
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/Package/MetadataUpdateTool.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/LanguageService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
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.
Thanks @raych1 for sending this PR to me and for putting the code together! But both the spec PR and Ben's PR are not merged and agreed by all. This PR is already imlementation. Should it pending on spec PR and Ben's PR be merged?
I was advised to first refer to spec PR for language integration details, and then refer to Ben's PR on integration, now refer to this PR. But all PRs are not agreed by all. I'm afraid I am quite confused and unclear on how to integrate with you at this point. Shanghai side work is pending on a clear agreement on integration.
We may like to see:
- an official and clear email, or a comment in this PR (rather than instant message 😢 ) to get it agreed by all stakeholders to show the integration approach for each language, as the design changed force and back.
- an example on how to integrate with AzureCLI if we are using scripts, see this comment: #12761 (comment). Shanghai side language owners may prefer script approach.
We are waiting for above information and for this PR to be approved by all stakeholders to start Shanghai side work.
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PackageUpdate/JavaPackageUpdate.cs
Outdated
Show resolved
Hide resolved
|
To @raych1: Shanghai mgmt sdk language owners prefer to use the script approach to integrate, as
We are waiting for this PR to be merged to start our Shanghai side work to integrate. |
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/IResponseFactory.cs
Outdated
Show resolved
Hide resolved
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.
To unblock the merge :), since I will be OOF tmr and might not get the chance to review latest changes. But please resolve my comments above as it may block shanghai side work.
…teTool.cs Co-authored-by: Copilot <[email protected]>
|
@haolingdong-msft @msyyc I'm working on taking over Ben's spec PR #12696 to come up with a new script interface design. In the meantime I've asked @raych1 to continue to use the existing |
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/SpecGenSdkConfigHelper.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PackageUpdate/DotNetPackageUpdate.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PackageUpdate/GoPackageUpdate.cs
Outdated
Show resolved
Hide resolved
@haolingdong-msft @msyyc I've added the configuration section and sample to the spec For the script contract, please refer to the last section in the original Word design spec. |
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
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Helpers/SpecGenSdkConfigHelper.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/LanguageService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/ProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/Package/MetadataUpdateTool.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/PackageUpdate/DotNetPackageUpdate.cs
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/LanguageService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/IProcessConfigurationService.cs
Outdated
Show resolved
Hide resolved
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Services/Languages/LanguageService.cs
Outdated
Show resolved
Hide resolved
weshaggard
left a comment
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.
One more clean-up but otherwise looks
tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/Package/MetadataUpdateTool.cs
Outdated
Show resolved
Hide resolved
…teTool.cs Co-authored-by: Wes Haggard <[email protected]>
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.
@haolingdong-msft @msyyc I've added the configuration section and sample to #12563 For the script contract, please refer to the last section in the original Word design spec.
Thanks @raych1 and @weshaggard for the clarification! We will use the word format design for contract.
Thanks @raych1 for the code. Leave two questions. Just want to make sure our script integrates with the code well. Thank you!
This is the design spec
Overview of the changes:
Standardized response and process handling:
ISpecGenSdkConfigHelperinterface for creating and executing repo-configured processes with standardized error handling and response formatting.Language-specific package update services:
ILanguagePackageUpdateinterface