Repository navigation
[AutoPR] test workflow - #40040
[AutoPR] test workflow#40040ZiWei Chen (kazrael2119) wants to merge 1 commit into
Conversation
|
Azure Pipelines: 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
Mgmt
ZiWei Chen (kazrael2119)
added
mgmt-review-needed
and removed
mgmt-review-needed
labels
github-actions
Bot
added
mgmt-review-in-progress
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR (@azure/arm-automation 11.0.0 → 12.0.0) introduces 4 findings: 1 tool issue (regressed import type → import for type-only imports in the API report) and 3 design concerns (breaking rename of AutomationClient.activityOperations to activity, void introduced into ScheduleOperationsOperations.createOrUpdate return type against type-safety guidance, and a major/breaking release shipped only ~2 months after the prior major). Package version, context file (automationContext.ts), and CHANGELOG.md versions are consistent (12.0.0). No alpha versions or CHANGELOG baseline issues detected.
📊 Structured Report
{"agent":"mgmt-reviewer","pr":40040,"summary":"issues_found","findings":[{"file":"sdk/automation/arm-automation/review/arm-automation-node.api.md","line":7,"issueType":"tool","category":"import-type-regression","description":"Type-only imports emitted as value imports instead of `import type`."},{"file":"sdk/automation/arm-automation/review/arm-automation-node.api.md","line":253,"issueType":"design","category":"breaking-rename","description":"AutomationClient.activityOperations renamed to activity (breaking client surface rename)."},{"file":"sdk/automation/arm-automation/review/arm-automation-node.api.md","line":2745,"issueType":"design","category":"type-safety","description":"ScheduleOperationsOperations.createOrUpdate return type changed to include void."},{"file":"sdk/automation/arm-automation/CHANGELOG.md","line":3,"issueType":"design","category":"frequent-breaking-release","description":"Major breaking release 12.0.0 shipped ~2 months after 11.0.0."}]}⚡ Benchmarked by Management Release Assistant
| // @public | ||
| export interface ScheduleOperationsOperations { | ||
| createOrUpdate: (resourceGroupName: string, automationAccountName: string, scheduleName: string, parameters: ScheduleCreateOrUpdateParameters, options?: ScheduleOperationsCreateOrUpdateOptionalParams) => Promise<Schedule | undefined>; | ||
| createOrUpdate: (resourceGroupName: string, automationAccountName: string, scheduleName: string, parameters: ScheduleCreateOrUpdateParameters, options?: ScheduleOperationsCreateOrUpdateOptionalParams) => Promise<Schedule | void>; |
There was a problem hiding this comment.
🔴 Design Concern — arm-automation-node.api.md:2745
ScheduleOperationsOperations.createOrUpdate return type changed from Promise<Schedule | undefined> to Promise<Schedule | void>. Per the type-safety guidelines, void should be avoided as (part of) the return type for create/update operations — callers get no usable result on this path.
Fix: Prefer a concrete/optional type (e.g., Promise<Schedule | undefined> as before, or a single Promise<Schedule>) instead of unioning with void. If generated from TypeSpec, adjust the operation's response modeling in the spec (client.tsp) and regenerate.
| import { AbortSignalLike } from '@azure/abort-controller'; | ||
| import { CancelOnProgress } from '@azure/core-lro'; | ||
| import { ClientOptions } from '@azure-rest/core-client'; | ||
| import { isRestError } from '@azure/core-rest-pipeline'; | ||
| import type { OperationOptions } from '@azure-rest/core-client'; | ||
| import type { OperationState } from '@azure/core-lro'; | ||
| import type { PathUncheckedResponse } from '@azure-rest/core-client'; | ||
| import type { Pipeline } from '@azure/core-rest-pipeline'; | ||
| import type { PollerLike } from '@azure/core-lro'; | ||
| import { OperationOptions } from '@azure-rest/core-client'; | ||
| import { OperationState } from '@azure/core-lro'; | ||
| import { PathUncheckedResponse } from '@azure-rest/core-client'; | ||
| import { Pipeline } from '@azure/core-rest-pipeline'; | ||
| import { PollerLike } from '@azure/core-lro'; | ||
| import { RestError } from '@azure/core-rest-pipeline'; | ||
| import type { TokenCredential } from '@azure/core-auth'; | ||
| import { TokenCredential } from '@azure/core-auth'; |
There was a problem hiding this comment.
🔴 Tool Issue — arm-automation-node.api.md:7-17
Several previously type-only imports (AbortSignalLike, CancelOnProgress, ClientOptions, OperationOptions, OperationState, PathUncheckedResponse, Pipeline, PollerLike, TokenCredential) changed from import type { ... } to import { ... } in this PR, while they are still only referenced as types in the report.
Fix: These should remain import type since only type positions are used; this looks like a regression in the generator's import-emission logic. Please report this in the generation tool repository.
| constructor(credential: TokenCredential, options?: AutomationClientOptionalParams); | ||
| constructor(credential: TokenCredential, subscriptionId: string, options?: AutomationClientOptionalParams); | ||
| readonly activityOperations: ActivityOperationsOperations; | ||
| readonly activity: ActivityOperationsOperations; |
There was a problem hiding this comment.
🔴 Design Concern — arm-automation-node.api.md:253
readonly activityOperations: ActivityOperationsOperations; was renamed to readonly activity: ActivityOperationsOperations;.
This is a breaking rename of a public client property, which is not recommended per the breaking-change guidelines (client surface member name changed).
Fix: Prepare a spec change to keep the original property name activityOperations (use @clientName/@access decorators as appropriate in client.tsp), then trigger SDK regeneration. If the rename is intentional, please explain the rationale and confirm sign-off from the API review board.
| @@ -1,4 +1,12 @@ | |||
| # Release History | |||
|
|
|||
| ## 12.0.0 (2026-09-22) | |||
There was a problem hiding this comment.
🔴 Design Concern — CHANGELOG.md:3
This PR releases 12.0.0 with multiple breaking changes only ~2 months after 11.0.0 (2026-07-27) was released. Per the guidelines, a major/breaking version released within 6 months of the previous major is not recommended, as frequent breakages hurt customers.
Fix: Please confirm whether these breaking changes (LRO signature change for CertificateOperationsOperations.createOrUpdate, return-type change for ScheduleOperationsOperations.createOrUpdate, and the activityOperations → activity rename) can be deferred/batched, or provide justification for shipping them so soon after the last major release.
github-actions
Bot
added
mgmt-review-in-progress
There was a problem hiding this comment.
Reviewed PR #40040 (@azure/arm-automation 11.0.0 → 12.0.0). Package version, context file version, and CHANGELOG.md are consistent, and the documented breaking changes (property rename activityOperations → activity, LRO signature change for CertificateOperationsOperations.createOrUpdate) are properly reflected in CHANGELOG.md. One public API design concern found: 1 finding (design).
📊 Structured Report
{"agent":"mgmt-reviewer","pr":40040,"summary":"issues_found","findings":[{"file":"sdk/automation/arm-automation/review/arm-automation-node.api.md","line":2745,"issueType":"design","category":"type-safety","description":"ScheduleOperationsOperations.createOrUpdate returns Promise<Schedule | void>; void should be avoided in create/update return types per guidelines."}]}⚡ Benchmarked by Management Release Assistant
| // @public | ||
| export interface ScheduleOperationsOperations { | ||
| createOrUpdate: (resourceGroupName: string, automationAccountName: string, scheduleName: string, parameters: ScheduleCreateOrUpdateParameters, options?: ScheduleOperationsCreateOrUpdateOptionalParams) => Promise<Schedule | undefined>; | ||
| createOrUpdate: (resourceGroupName: string, automationAccountName: string, scheduleName: string, parameters: ScheduleCreateOrUpdateParameters, options?: ScheduleOperationsCreateOrUpdateOptionalParams) => Promise<Schedule | void>; |
There was a problem hiding this comment.
🔴 Design Concern — ScheduleOperationsOperations.createOrUpdate return type changed to Promise<Schedule | void>.
Per the type-safety guideline, void should be avoided as (part of) the return type for create/update/get/list operations — it's appropriate mainly for delete/upgrade/reset-style actions. Returning Schedule | void forces callers to narrow/cast even on success paths.
Fix: This stems from the spec (client.tsp); update the TypeSpec/Swagger definition so createOrUpdate returns a concrete Schedule (or a discriminated union) rather than including void, then regenerate the SDK.
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists