Fabric Mirroring Review Fixes#43298
Conversation
|
Addresses 4 ARM API review comments on FabricMirroringSettings from PR #39083:
Only Fabric Mirroring changes. ReaderEndpoint/Rename Server not addressed here. |
Next Steps to MergeNext steps that must be taken to merge this PR:
Important checks have failed. As of today they are not blocking this PR, but in near future they may. Addressing the following failures is highly recommended:
Comment generated by summarize-checks workflow run. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
Comment generated by After APIView workflow run. |
|
@mohitsinha-ms please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
| SegmentName = "fabricMirroringSettings", | ||
| NamePattern = "^[a-zA-Z0-9-]{3,24}$" | ||
| NamePattern = "", | ||
| Type = FabricMirroringSettingsName |
There was a problem hiding this comment.
Review: Resource accepted arbitrary 3-24 char names but is a singleton. Name MUST be lowercase default with enum.
Fix: Added FabricMirroringSettingsName union with Default: "default", updated ResourceNameParameter with Type, all examples use lowercase default.
| } | ||
|
|
||
| #suppress "@azure-tools/typespec-azure-resource-manager/no-resource-delete-operation" "FIXME: Update justification, follow aka.ms/tsp/conversion-fix for details" | ||
| #suppress "@azure-tools/typespec-azure-resource-manager/no-resource-delete-operation" "FabricMirroringSettings is a server-intrinsic singleton whose lifecycle is tied to the parent server. Settings are toggled via PUT with state Enabled/Disabled rather than created or deleted independently." |
There was a problem hiding this comment.
Addresses: BLOCKING — Suppression Criteria / RPC-Delete-V1-05 (Fix FIXME suppression)
Review: Placeholder FIXME suppression masks real contract gap (no DELETE on proxy resource).
Fix: Replaced with definitive justification. Same pattern as AdvancedThreatProtection and Configuration which also suppress DELETE.
| ArmAsyncOperationHeader<FinalResult = FabricMirroringSetting> & | ||
| Azure.Core.Foundations.RetryAfterHeader | ||
| > | ArmAcceptedLroResponse | ||
| > |
There was a problem hiding this comment.
Review: Async PUT returning 202 is deprecated for new resource types.
Fix: Removed ArmAcceptedLroResponse + suppression. PUT returns 200/201 only with async LRO headers.
|
|
||
| @doc("List Fabric Mirroring settings by server.") | ||
| listByServer is Azure.ResourceManager.Legacy.ArmListSinglePageByParent<FabricMirroringSetting>; | ||
| listByServer is ArmResourceListByParent<FabricMirroringSetting>; |
There was a problem hiding this comment.
Review: ArmListSinglePageByParent with nextLink is inconsistent.
Fix: Replaced with ArmResourceListByParent to emit x-ms-pageable.
There was a problem hiding this comment.
Does your API support pagination? Does this change match your API definition?
There was a problem hiding this comment.
FabricMirroringSettings is a singleton (only 'default' allowed), so the list always returns at most 1 item and pagination is not functionally needed.
However, the ARM reviewer flagged that the previous ArmListSinglePageByParent was inconsistent -- it exposed nextLink in the response schema without x-ms-pageable.
So I thought Using ArmResourceListByParent resolves this inconsistency and matches the pattern used by all other list operations in this service (AdvancedThreatProtection, Configuration, Database, etc.). The service should return nextLink: null in responses.
Choose a PR Template
Switch to "Preview" on this description then select one of the choices below.
Click here to open a PR for a Data Plane API.
Click here to open a PR for a Control Plane (ARM) API.
Click here to open a PR for only SDK configuration.