Skip to content
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

mgmt+tsp, brownfield for vmware/avs #2803

Open
weidongxu-microsoft opened this issue Jun 5, 2024 · 36 comments
Open

mgmt+tsp, brownfield for vmware/avs #2803

weidongxu-microsoft opened this issue Jun 5, 2024 · 36 comments
Assignees
Labels
Mgmt This issue is related to a management-plane library.

Comments

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jun 5, 2024

vmware/avs already GAed with Swagger

service moved to TypeSpec on 2023-09-01
Azure/azure-rest-api-specs#28023
(at the time of the PR, the breaking changes CI having problem and not picking up the breaks)

service should name the folder as suffix ".Management", see https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/typespec-structure-guidelines.md#service-folder-structure

One remaining issue on typespec-azure

WIP

@weidongxu-microsoft weidongxu-microsoft added the Mgmt This issue is related to a management-plane library. label Jun 5, 2024
@weidongxu-microsoft weidongxu-microsoft self-assigned this Jun 5, 2024
@weidongxu-microsoft weidongxu-microsoft changed the title mgmt+tsp, brownfield of vmware/avs mgmt+tsp, brownfield for vmware/avs Jun 5, 2024
@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 5, 2024

REST API breaks

Need confirmation from service whether this is intended.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 5, 2024

API breaks

This is hard to keep non-breaking change in SDK.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 5, 2024

SDK models

  • x-ms-client-flatten
  • mismatch on model names / property names

Mostly addressable in "client.tsp"
Azure/azure-rest-api-specs#29308

Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic
Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 5, 2024

SDK operation groups

  • Lots of @operationId, which is not supported in SDK emitter (they are only for typespec-autorest emitter)

WIP on whether we need to modify the interface structure in "route.tsp"
e.g. Azure/azure-rest-api-specs@7105e2d

(alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

@weidongxu-microsoft

This comment was marked as outdated.

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

service should name the folder as suffix ".Management", see https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/typespec-structure-guidelines.md#service-folder-structure

I thought it was the RP name. Yes, you can change it to align with the guidance.

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition.

https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

This was unintentional. It should be changed back to int64.

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

This was an approved breaking change. model WorkloadNetwork is a @singleton and this is how it is modeled in TypeSpec.

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

The properties are spread in. This is how it is done in TypeSpec.

model ManagementCluster {
  ...CommonClusterProperties;
}
model ClusterProperties {
  ...CommonClusterProperties;
}

I'm not sure of another way to do it in TypeSpec. For example, this does not work here:
image

Not having the inheritance modeled is breaking, but seams acceptable.

@cataggar
Copy link
Member

cataggar commented Jun 5, 2024

SDK operation groups

  • Lots of @operationId, which is not supported in SDK emitter (they are only for typespec-autorest emitter)

WIP on whether we need to modify the interface structure in "route.tsp" e.g. Azure/azure-rest-api-specs@7105e2d

(alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

I did a lot of work to make the operationIds match in the generated OpenAPI. Are there operationIds that do not match? Are you generating the SDK from the OpenAPI spec or directly from TypeSpec?

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 6, 2024

SDK operation groups

  • Lots of @operationId, which is not supported in SDK emitter (they are only for typespec-autorest emitter)

WIP on whether we need to modify the interface structure in "route.tsp" e.g. Azure/azure-rest-api-specs@7105e2d
(alternative is write all the APIs explicitly in client.tsp, which likely be hard to maintain)

I did a lot of work to make the operationIds match in the generated OpenAPI. Are there operationIds that do not match? Are you generating the SDK from the OpenAPI spec or directly from TypeSpec?

@operationId is from typespec-openapi lib
https://typespec.io/docs/libraries/openapi/reference/decorators#@TypeSpec.OpenAPI.operationId

Java SDK would be generated directly from TypeSpec (and hence won't use the openapi lib or any decorator within). -- any SDK emitter that is ready to generate from TypeSpec would generate SDK from TypeSpec

The @operationId is required anyway, since MS Learn still generate REST API doc from Swagger.
Just SDK emitter need a different way to solve the operation group.
Would you mind we adjust the interface in route.tsp (we would also get a acknowledge from Alan/Mark before doing it)?

PS: we are making corrections to operation name (the part after _ in operationId in Swagger) as well. This is something we can do in client.tsp https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L44-L56

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 6, 2024

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition.

https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

But, unfortunately, it is already in Swagger for a very long time, and lots of SDK have been shipped with that.

We will discuss the break within SDK devs first, see if we accept it, or try to keep backward-compatible. Do you mind, if we modify the operation to make it same as your older Swagger, if we decide to keep backward-compatible?

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 6, 2024

This was unintentional. It should be changed back to int64.

PR to change back to int64 Azure/azure-rest-api-specs#29351
Please approve the PR if you are OK with it.

@cataggar
Copy link
Member

cataggar commented Jun 6, 2024

We do not actually want the operation API in our spec or SDKs. I've never understood why it is required. Anyhow, in TypeSpec we are using the standard typespec-azure-core definition.
https://github.com/Azure/azure-rest-api-specs/blob/32f566eb38af9593cfc1ed183362471c50d67fb2/specification/vmware/Microsoft.AVS/routes.tsp#L15

But, unfortunately, it is already in Swagger for a very long time, and lots of SDK have been shipped with that.

We will discuss the break within SDK devs first, see if we accept it, or try to keep backward-compatible. Do you mind, if we modify the operation to make it same as your older Swagger, if we decide to keep backward-compatible?

We are using the standard Azure.ResourceManager.Operations. We never wanted the API in our SDKs. We do not want to maintain a non-standard version of it.

@cataggar
Copy link
Member

cataggar commented Jun 6, 2024

Hi @weidongxu-microsoft , an engineer working on the Azure CLI noticed that ManagementCluster used to require clusterSize, but no longer does. That was non intentional and should be changed back. Do you see the impact in this generated Java SDK?

https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/resource-manager/Microsoft.AVS/stable/2023-03-01/vmware.json#L6106-L6110

    "ManagementCluster": {
      "type": "object",
      "description": "The properties of a management cluster",
      "required": [
        "clusterSize"
      ],

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 7, 2024

Hi @weidongxu-microsoft , an engineer working on the Azure CLI noticed that ManagementCluster used to require clusterSize, but no longer does. That was non intentional and should be changed back. Do you see the impact in this generated Java SDK?

https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/resource-manager/Microsoft.AVS/stable/2023-03-01/vmware.json#L6106-L6110

    "ManagementCluster": {
      "type": "object",
      "description": "The properties of a management cluster",
      "required": [
        "clusterSize"
      ],

As mentioned in the discription of this issue, the BreakingChanges CI is broken when your merge that migration PR. It really not able to report anything useful. (tooling team already got notified and fixing/fixed the problem)

This part of Swagger is pretty tricky. You can see that the clusterSize is defined in CommonClusterProperties, but the required=["clusterSize"] is defined in ManagementCluster. SDK codegen/emitter may not able to handle this case as what you expect (as most langugages model this allOf as superclass/subclass relationship, and subclass usually won't touch property/variable from its superclass).

I've looked at existing .NET code (2021-06-01? form the last commit), appears not a required as well. (it is int? with a default value)
https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/avs/Microsoft.Azure.Management.Avs/src/Generated/Models/ManagementCluster.cs#L39

@weidongxu-microsoft
Copy link
Member Author

And we are still in the progress of trying to get all the potential breaks.

Here is another one, please let us know if we want to revert this int32 back to int64

@cataggar
Copy link
Member

cataggar commented Jun 7, 2024

And we are still in the progress of trying to get all the potential breaks.

Here is another one, please let us know if we want to revert this int32 back to int64

Yes, also unintentional and should be changed back.

@cataggar
Copy link
Member

cataggar commented Jun 7, 2024

This part of Swagger is pretty tricky. You can see that the clusterSize is defined in CommonClusterProperties, but the required=["clusterSize"] is defined in ManagementCluster. SDK codegen/emitter may not able to handle this case as what you expect (as most langugages model this allOf as superclass/subclass relationship, and subclass usually won't touch property/variable from its superclass).

I've looked at existing .NET code (2021-06-01? form the last commit), appears not a required as well. (it is int? with a default value) https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/avs/Microsoft.Azure.Management.Avs/src/Generated/Models/ManagementCluster.cs#L39

You are right. Same with JavaScript here, it is clusterSize?: number;. I think it is best to leave it the way it is.

@weidongxu-microsoft
Copy link
Member Author

Yes, also unintentional and should be changed back.

PR to fix Azure/azure-rest-api-specs#29395

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 11, 2024

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate

This would cause some breaks to Go (and probably other languages as well).

Please let us know if this is intended. If yes, any reason?

@raych1
Copy link
Member

raych1 commented Jun 12, 2024

SDK models

  • x-ms-client-flatten
  • mismatch on model names / property names

Mostly addressable in "client.tsp" Azure/azure-rest-api-specs#29308

Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 12, 2024

SDK models

  • x-ms-client-flatten
  • mismatch on model names / property names

Mostly addressable in "client.tsp" Azure/azure-rest-api-specs#29308
Important: AddonProperties cannot be x-ms-client-flatten as it is polymorphic Should TCGC handle this (i.e., set flatten=false, whatever the @flattenProperty)?

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`

In TypeSpec, it can be renamed in client.tsp
https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L59-L64

For Swagger, it may had to be done via directive.
Azure/azure-rest-api-specs#29413

@weidongxu-microsoft
Copy link
Member Author

  • rename of List responses from ##List to ##ListResult

This can be handled by using a customized ResourceList template, e.g. https://github.com/Azure/azure-rest-api-specs/pull/29408/files

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 12, 2024

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated

The x-ms-mutability comes from ProxyResource in lib

Is it intended?

@cataggar
Copy link
Member

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated

The x-ms-mutability comes from ProxyResource in lib

Is it intended?

It is not intended.

@cataggar
Copy link
Member

  • rename of List responses from ##List to ##ListResult

This can be handled by using a customized ResourceList template, e.g. https://github.com/Azure/azure-rest-api-specs/pull/29408/files

I approved that draft. That looks like a good way to restore name compatibility.

@cataggar
Copy link
Member

@weidongxu-microsoft , the following two properties need to be renamed back to current name in client.tsp.

`PrivateCloud.Identity`
`PrivateCloudUpdate.Identity`

In TypeSpec, it can be renamed in client.tsp https://github.com/Azure/azure-rest-api-specs/blob/1418642cf8a57954c781c26c759fd4aef39a1287/specification/vmware/Microsoft.AVS/client.tsp#L59-L64

For Swagger, it may had to be done via directive. Azure/azure-rest-api-specs#29413

We do wish that the generated SDKs use the standard identity types. We support system identity now, but will support user managed identity in the future.

@cataggar
Copy link
Member

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate

This would cause some breaks to Go (and probably other languages as well).

Please let us know if this is intended. If yes, any reason?

No, not intended.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 13, 2024

There is certain change to "x-ms-mutability", e.g. properties in Addon changed to "read", "create" -- meaning this proxy resource cannot be updated
The x-ms-mutability comes from ProxyResource in lib
Is it intended?

It is not intended.

The unreleased typespec-azure-resource-manager appears have removed this @visibility
https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-resource-manager/lib/models.tsp#L62-L66

typespec-next

We probably just wait for the new lib to be released.

@weidongxu-microsoft
Copy link
Member Author

I was told by Go that there was multiple places in PATCH, that previously request body is e.g. WorkloadNetworkSegment (same model as in PUT) being changed to WorkloadNetworkSegmentUpdate
This would cause some breaks to Go (and probably other languages as well).
Please let us know if this is intended. If yes, any reason?

No, not intended.

PR to move it back Azure/azure-rest-api-specs#29392

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 13, 2024

We do wish that the generated SDKs use the standard identity types. We support system identity now, but will support user managed identity in the future.

ManagedServiceIdentity has one more property (userAssignedIdentities) compared to SystemAssignedServiceIdentity. Other properties appear to be exactly same.

Therefore, I think service should be able to evolve from SystemAssignedServiceIdentity to ManagedServiceIdentity.

Though for SDK, we probably would continue do this rename, so that user would see a same class/model (with new property userAssignedIdentities), before/after the evolution.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Jun 17, 2024

All Swagger related PR should have been merged.

Allen upgraded spec repo to 0.57 as well (it includes the Swagger change via typespec-azure-resource-manager).

Swagger should be completed.

Remain issue to TypeSpec (on SDK emitter) is the operation group that is still in discussion https://gist.github.com/weidongxu-microsoft/fa420c35cf0611a62a8d155771dd3425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

No branches or pull requests

3 participants