-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Validate resolveArmResources API coverage for resource detection replacement #54204
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
base: main
Are you sure you want to change the base?
Conversation
…ations Co-authored-by: live1206 <[email protected]>
|
@copilot could you set up a test to demonstrate the bug in |
…gletons Co-authored-by: live1206 <[email protected]>
Added test file The test file includes:
|
|
@copilot apart from the failing case you found before. |
Co-authored-by: live1206 <[email protected]>
Added comprehensive test file Results Summary:
Known Bug: Multiple singleton resources with different |
@copilot This looks good, now replace the logic in https://github.com/Azure/azure-sdk-for-net/blob/main/eng/packages/http-client-csharp-mgmt/emitter/src/resource-detection.ts with |
Co-authored-by: live1206 <[email protected]>
I investigated using Discovered bugs in
The validation test file These bugs need to be fixed in the TypeSpec ARM library before the replacement can be completed. Should I file issues at https://github.com/Azure/typespec-azure? |
@copilot Apart from the first case, which you already created a test for it. Could you point to the existing TypeSpec under https://github.com/Azure/azure-sdk-for-net/tree/main/eng/packages/http-client-csharp-mgmt/generator/TestProjects/Local/Mgmt-TypeSpec? If there is no existing TypeSpec, could you create a emitter test for it like the first case? |
…g TypeSpec reference Co-authored-by: live1206 <[email protected]>
Added tests demonstrating bugs 2-5 in commit 28ae6a0. Existing TypeSpec reference: New test cases in
|
| @@ -0,0 +1,162 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot merge this file into resolve-arm-resources-validation.test.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged resolve-arm-resources-bug.test.ts into resolve-arm-resources-validation.test.ts in commit 6a123ff. The validation test file now contains all bug demonstration tests in a single location.
Co-authored-by: live1206 <[email protected]>
eng/packages/http-client-csharp-mgmt/emitter/test/resolve-arm-resources-validation.test.ts
Show resolved
Hide resolved
eng/packages/http-client-csharp-mgmt/emitter/test/resolve-arm-resources-validation.test.ts
Show resolved
Hide resolved
eng/packages/http-client-csharp-mgmt/emitter/test/resolve-arm-resources-validation.test.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| /** An Employee resource with grandparent */ | ||
| @subscriptionResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there is no need to mark any resource but the grandparent as a subscription resource
| } | ||
|
|
||
| /** A Department parent resource */ | ||
| @tenantResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment. Only the grandparent resource needs to be decorated with @subscriptionResource
| return "Tenant"; | ||
| } | ||
|
|
||
| it("resource group resource - basic validation", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicative of existing tests in the typespec-azure repo, but doesn't hurt to have it here as well
| ); | ||
| }); | ||
|
|
||
| it("resource scope determined from Get method (SubscriptionLocationResource parent)", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is duplicative of existing tests in the typespec-azure repo, but no harm in including it here
| ); | ||
| }); | ||
|
|
||
| it("API returns operation details for lifecycle operations", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicative of existing tests in the typespec-azure repo, but no harm in including it here
| `; | ||
|
|
||
| /** | ||
| * BUG 2: Singleton child resources return incorrect resourceInstancePath. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good catch
markcowl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments
resource-detection.tsresolveArmResourcesAPI from@azure-tools/typespec-azure-resource-managerresolveArmResourcesAPI behavior and document findingsresolve-arm-resources-validation.test.tswith:resource-detection.test.tsusingresolveArmResourcesAPIbar.tsp)Summary
The
resolveArmResourcesAPI (v0.62.0) from@azure-tools/typespec-azure-resource-managerhas 5 confirmed bugs that prevent direct replacement of the decorator-based resource detection logic:@singletonkeys are not distinguishedresourceInstancePath@parentResourcedecoratorNext Steps
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.