Skip to content

Instantly share code, notes, and snippets.

@weidongxu-microsoft
Last active June 21, 2024 09:06
Show Gist options
  • Save weidongxu-microsoft/fa420c35cf0611a62a8d155771dd3425 to your computer and use it in GitHub Desktop.
Save weidongxu-microsoft/fa420c35cf0611a62a8d155771dd3425 to your computer and use it in GitHub Desktop.
Discussion about grouping operations

TCGC

TCGC, by default, groups operations via TypeSpec Interfaces.

If @operationGroup is specified in client.tsp, it would group operations from Interfaces with @operationGroup in client.tsp instead.

Management - brownfield

Swagger in Management usually groups many different resources in a single resource group, via "operationId".

When migrating to TypeSpec, service need to write mutliple Interface for these operations. Service would use @operationId (from typespec-openai lib) to keep "operationId" unchanged in Swagger.

SDK emitter does not read @operationId decorator.

Known approach to group operations

As DPG, SDK dev would add @operationGroup interface <ClientOperationGroup> {} to client.tsp

However, due to an issue about armRenameListByOperation decorator over ArmResourceListByParent, 2 operations of ArmResourceListByParent with resoures of same parent resource cannot be written in a same Interface. (discussion at Teams; reporduction at playground; issue to typespec-azure)

Possible solutions

1. Do nothing

Accept the break to stable SDK. Usually the impact would be at the scale of 10s of APIs.

E.g. there is about 40 of Workloadnetworks_<operation> in vmware. They would scatter to different operation groups as e.g. WorkloadNetworkSegments, WorkloadNetworkDhcpConfigurations, etc.

2. Remove the armRenameListByOperation decorator

In Teams chat, Mark appears OK to remove the decorator.

Currently there is about 152+ places that ArmResourceListByParent is used. We need to modify all of them, so that the operation name stays same before/after the removal (put it another way, Swagger should be same, before/after the removal). Then remove the decorator. This will take time and efforts.

After that, we would be free to group it either in Interface in routes.tsp or in client.tsp

Though, service would need to maintain this client.tsp, e.g. after they add new Interface.

3. Have @operationGroup override the operation group name

Add an overload of @operationGroup as @operationGroup(operationGroupName: string). So that it can be specified on multiple Interface, and merge them to a single operation group. E.g.

@@operationGroup(WorkloadNetworks);
@@operationGroup(WorkloadNetworkSegments, "WorkloadNetworks");
@@operationGroup(WorkloadNetworkDhcpConfigurations, "WorkloadNetworks");

The logic in TCGC would be complicated.

@tadelesh
Copy link

i vote for 2nd bc it is a thorough solution though legacy typespec fix is annoying

@msyyc
Copy link

msyyc commented Jun 12, 2024

Vote 2 or at least solution similar 2 like provide config to close armRenameListByOperation

@qiaozha
Copy link

qiaozha commented Jun 12, 2024

vote 2, I personally think ArmResourceListByParent is not a common pattern, maybe we should also reconsider about rename/redesign the operation template?

@weidongxu-microsoft
Copy link
Author

weidongxu-microsoft commented Jun 12, 2024

vote 2, I personally think ArmResourceListByParent is not a common pattern, maybe we should also reconsider about rename/redesign the operation template?

It appears to be the standard template for child resource, and top-level resource as well (I guess for top-level resource, its parent would be resource group).

The only other commonly used List would be ArmListBySubscription

@weidongxu-microsoft
Copy link
Author

weidongxu-microsoft commented Jun 21, 2024

If we only solve 2 (task Azure/typespec-azure#1052), the minimal client.tsp would be like below

@client({
  name: "AvsClient", 
  service: Microsoft.AVS
})
@useDependency(Microsoft.AVS.Versions.v2023_09_01)
namespace Customizations;

@operationGroup
@armResourceOperations
interface Operations extends Microsoft.AVS.Operations {};

@operationGroup
@armResourceOperations
interface PrivateClouds extends Microsoft.AVS.PrivateClouds {};

@operationGroup
@armResourceOperations
interface WorkloadNetworks extends Microsoft.AVS.WorkloadNetworks {
  #suppress "@azure-tools/typespec-azure-core/no-operation-id" "Can not change existing operationId."
  listSegments is Microsoft.AVS.WorkloadNetworkSegments.list;

  #suppress "@azure-tools/typespec-azure-core/no-operation-id" "Can not change existing operationId."
  getSegment is Microsoft.AVS.WorkloadNetworkSegments.get;

  #suppress "@azure-tools/typespec-azure-core/no-operation-id" "Can not change existing operationId."
  #suppress "@azure-tools/typespec-azure-core/invalid-final-state" "MUST CHANGE ON NEXT UPDATE"
  createSegment is Microsoft.AVS.WorkloadNetworkSegments.create;

  #suppress "@azure-tools/typespec-azure-core/no-operation-id" "Can not change existing operationId."
  updateSegment is Microsoft.AVS.WorkloadNetworkSegments.update;

  #suppress "@azure-tools/typespec-azure-core/no-operation-id" "Can not change existing operationId."
  #suppress "@azure-tools/typespec-azure-resource-manager/arm-delete-operation-response-codes"
  deleteSegment is Microsoft.AVS.WorkloadNetworkSegments.deleteSegment;
};

(there will be 30 more operations in interface WorkloadNetworks, and there will be a few more @operationGroup interfaces)

And there is currently 2 problems

  1. TCGC does not recognize this namespace Customizations as ARM client Azure/typespec-azure#1049 (fix PR ready)
  2. All the #suppress need to be duplicated in client.tsp -- this not a blocker, but against DRY and very tedious

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment