-
Notifications
You must be signed in to change notification settings - Fork 66
Fix issues when we have singleton resource with a customized name #3594
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?
Changes from all commits
6940e1a
c4d1267
b9a7446
451d7b9
290dd82
f5f5910
155ce30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| # Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking | ||
| changeKind: fix | ||
| packages: | ||
| - "@azure-tools/typespec-azure-resource-manager" | ||
| --- | ||
|
|
||
| Fix issues in `resolveArmResources` when we have singleton resource with a customized name |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,6 +167,8 @@ interface ResolvedResourceOperations { | |
| parent?: ResolvedResource; | ||
| /** The scope of this resource */ | ||
| scope?: string; | ||
| /** For singleton resources, the names that can be used to reference the resource */ | ||
| singletonResourceNames?: string[]; | ||
| } | ||
| /** Resolved operations, including operations for non-arm resources */ | ||
| export interface ResolvedResource { | ||
|
|
@@ -190,6 +192,8 @@ export interface ResolvedResource { | |
| parent?: ResolvedResource; | ||
| /** The scope of this resource */ | ||
| scope?: string | ResolvedResource; | ||
| /** For singleton resources, the names that can be used to reference the resource */ | ||
| singletonResourceNames?: string[]; | ||
| } | ||
|
|
||
| /** Description of the resource type */ | ||
|
|
@@ -621,15 +625,20 @@ function getResourceScope( | |
| return undefined; | ||
| } | ||
|
|
||
| function isVariableSegment(segment: string): boolean { | ||
| return (segment.startsWith("{") && segment.endsWith("}")) || segment === "default"; | ||
| function isVariableSegment(segment: string, singletonResourceName?: string): boolean { | ||
| return (segment.startsWith("{") && segment.endsWith("}")) || segment === singletonResourceName; | ||
| } | ||
|
|
||
| function getResourceInfo( | ||
| program: Program, | ||
| operation: ArmResourceOperation, | ||
| singletonResourceName: string | undefined, | ||
| ): ResolvedResourceInfo | undefined { | ||
| const pathInfo = getResourcePathElements(operation.httpOperation.path, operation.kind); | ||
| const pathInfo = getResourcePathElements( | ||
| operation.httpOperation.path, | ||
| operation.kind, | ||
| singletonResourceName, | ||
| ); | ||
| if (pathInfo === undefined) return undefined; | ||
| return { | ||
| ...pathInfo, | ||
|
|
@@ -640,6 +649,7 @@ function getResourceInfo( | |
| export function getResourcePathElements( | ||
| path: string, | ||
| kind: ArmOperationKind, | ||
| singletonResourceName?: string, | ||
| ): ResourcePathInfo | undefined { | ||
| const segments = path.split("/").filter((s) => s.length > 0); | ||
| const providerIndex = segments.findLastIndex((s) => s === "providers"); | ||
|
|
@@ -652,7 +662,15 @@ export function getResourcePathElements( | |
| break; | ||
| } | ||
|
|
||
| if (i + 1 < segments.length && isVariableSegment(segments[i + 1])) { | ||
| // if the next segment is the last segment | ||
| if ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we change this logic, we will fix the issues below - it makes sense that the singleton resource name should just always be checked when looking for a variable segment. I think there is still a case to think about, when multiple resources in the resource type are singletons |
||
| i + 1 === segments.length - 1 && | ||
| isVariableSegment(segments[i + 1], singletonResourceName) | ||
| ) { | ||
| typeSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i + 1]); | ||
| } else if (i + 1 < segments.length && isVariableSegment(segments[i + 1])) { | ||
| typeSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i + 1]); | ||
|
|
@@ -862,9 +880,10 @@ export function resolveArmResourceOperations( | |
|
|
||
| if (armOperation === undefined) continue; | ||
| armOperation.kind = operation.kind; | ||
| const singletonResourceName = getSingletonResourceKey(program, resourceType); | ||
|
|
||
| armOperation.resourceModelName = operation.resource?.name ?? resourceType.name; | ||
| const resourceInfo = getResourceInfo(program, armOperation); | ||
| const resourceInfo = getResourceInfo(program, armOperation, singletonResourceName); | ||
| if (resourceInfo === undefined) continue; | ||
| armOperation.name = operation.name; | ||
| armOperation.resourceKind = operation.resourceKind; | ||
|
|
@@ -893,6 +912,8 @@ export function resolveArmResourceOperations( | |
| resourceType: resourceInfo.resourceType, | ||
| resourceInstancePath: resourceInfo.resourceInstancePath, | ||
| resourceName: resourceInfo.resourceName, | ||
| singletonResourceNames: | ||
| singletonResourceName !== undefined ? [singletonResourceName] : undefined, | ||
| operations: { | ||
| lifecycle: { | ||
| read: undefined, | ||
|
|
||
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 we want to do a lowercase transform over both the segment and the name
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.
Also, I think we could do this over an array of singleton resource names, but this isn't required for this PR, we cna add it when we add singleton recognition for union and enum resource name types
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 we should also continue to use 'default' as a singleton resource name, in addition to the singleton resource name from getSingletonKey