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

tsp, TCGC common layer, adopt model types #2698

Merged
merged 114 commits into from
May 15, 2024

Conversation

haolingdong-msft
Copy link
Member

@haolingdong-msft haolingdong-msft commented Apr 16, 2024

API view with diff comparing this PR's generated code for typespec-tests with main branch:
https://apiview.dev/Assemblies/Review/e0cf256f814e456bac0f0d751bd4aa9f/d08e9493669445c4bf9916c090262bd5?diffRevisionId=b988ab5926314dc0a7d226750d035115&doc=False&diffOnly=True

Note this:
Currently TCGC treated enum as fixed enum, and this can cause breaking changes in mgmt plane sdk. typespec-azure will fix the tsp definition in 0.42.
image

Fix #2564

Will create issues for below follow up items and create seperate PR to support:

haolingdong-msft and others added 6 commits May 8, 2024 11:57
# Conflicts:
#	typespec-extension/changelog.md
#	typespec-extension/package-lock.json
#	typespec-extension/package.json
#	typespec-tests/package.json
#	typespec-tests/src/main/java/com/cadl/armresourceprovider/models/ManagedIdentityType.java
#	typespec-tests/src/main/java/com/cadl/flatten/implementation/models/SendLongRequest.java
#	typespec-tests/src/main/java/com/cadl/flatten/implementation/models/UploadTodoRequest.java
@haolingdong-msft
Copy link
Member Author

haolingdong-msft commented May 10, 2024

Our build passes at version "@azure-tools/typespec-client-generator-core": "0.42.0", but encouter generation error at version "@azure-tools/typespec-client-generator-core": "0.42.1".

Seems TCGC has issue on version 0.42.1 which causes the build failure: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3777204&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=50b9345f-2a7f-5980-cb0f-1a8f9fb09dd0&l=4528

Fixed the TCGC version to 0.42.0 currently to unblock Java CI.

@tadelesh @iscai-msft

@tadelesh
Copy link
Member

Our build passes at version "@azure-tools/typespec-client-generator-core": "0.42.0", but encouter generation error at version "@azure-tools/typespec-client-generator-core": "0.42.1".

Seems TCGC has issue on version 0.42.1 which causes the build failure: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3777204&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=50b9345f-2a7f-5980-cb0f-1a8f9fb09dd0&l=4528

Fixed the TCGC version to 0.42.0 currently to unblock Java CI.

@tadelesh @iscai-msft

0.42.1 should be a wrong hotfix release. we are trying to release another 0.42.2.

# Conflicts:
#	typespec-extension/src/code-model-builder.ts
@haolingdong-msft
Copy link
Member Author

haolingdong-msft commented May 11, 2024

Remaining issues on this PR:

  1. TCGC 0.42.2 released, I will integrate with the latest version to see if there is any behavior change.
  2. Mgmt case error after upgrade to 0.42: tsp, TCGC common layer, adopt model types #2698 (comment), Xiaofei is working on this. But we have pending discussion issue on TCGC(see item 3)

Three pending discussion issues:

  1. Type of model property/enum member when it is defined as constant number: Type of model property/enum member when it is defined as constant number microsoft/typespec#3314
  2. when should standalone models be generated: [TCGC][Discussion] model types, when should standalone models be generated typespec-azure#823
  3. ARM has definition: model UserAssignedIdentities is Record<UserAssignedIdentity>; But for model A is Record<> case, TCGC will return model A with additional property, this will be potential breaking changes in Mgmt plane, as current swagger generated code will use Map directly, instead of model with additonal properties. Should TCGC handle the
    UserAssignedIdentities specially or should ARM change the definition? [TCGC] [ARM] Expected client code on definition model UserAssignedIdentities is Record<UserAssignedIdentity>;  typespec-azure#825

/cc @iscai-msft @tadelesh @lmazuel

@XiaofeiCao
Copy link
Contributor

XiaofeiCao commented May 11, 2024

  1. Mgmt case error after switching to 0.42: tsp, TCGC common layer, adopt model types #2698 (comment), Xiaofei is working on this.

Offline synced with Haoling. The extra generated UserAssignedIdentities class is due to that we switched to using TCGC's result, and in 0.42, tsp-arm introduced a new model UserAssignedIdentities is Record<UserAssignedIdentity>, this will cause TCGC to generate a model A if model A is Record<>.

Though, tsp-arm themselves discourage use of model is Record: https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-record#-incorrect-1

@weidongxu-microsoft We probably need a special handling for UserAssignedIdentity, or ARM, in case of breaking changes to existing mgmt libs.

Raised an issue here: Azure/typespec-azure#824

# Conflicts:
#	typespec-extension/changelog.md
#	typespec-extension/package-lock.json
#	typespec-extension/package.json
#	typespec-tests/package.json
#	typespec-tests/tsp/union.tsp
# Conflicts:
#	typespec-extension/changelog.md
#	typespec-extension/package-lock.json
#	typespec-extension/package.json
#	typespec-extension/src/code-model-builder.ts
#	typespec-tests/package.json
#	typespec-tests/tsp/internal.tsp
@haolingdong-msft haolingdong-msft merged commit 82c3614 into Azure:main May 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dpg, integrate TCGC getAllModels - basic support
6 participants