-
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?
Conversation
commit: |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
| instanceSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i + 1]); | ||
| } else if (i + 1 === segments.length) { | ||
| if (i + 1 === segments.length) { |
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 don't think this is safe. We should look for the model being marked as a singleton.
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 made some updates around this part, please take a look again.
| if (i + 1 === segments.length - 1 && isVariableSegment(segments[i + 1], singletonResourceName)) { | ||
| typeSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i]); | ||
| instanceSegments.push(segments[i + 1]); |
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.
so we only consider the singletonResourceName when the current segment is the last segement.
| path: "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{providerName}/{resourceType}/{resourceName}/{childResourceType}/{childResourceName}/providers/Microsoft.Bar/bars/{barName}/basses/default/actionName/doSomething/doSomethingElse/andAnotherThing", | ||
| kind: "read", | ||
| singletonResourceName: "default", | ||
| expected: { | ||
| resourceType: { | ||
| provider: "Microsoft.Bar", | ||
| types: ["bars", "basses"], | ||
| types: ["bars"], | ||
| }, | ||
| resourceInstancePath: | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{providerName}/{resourceType}/{resourceName}/{childResourceType}/{childResourceName}/providers/Microsoft.Bar/bars/{barName}/basses/default", | ||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/{providerName}/{resourceType}/{resourceName}/{childResourceType}/{childResourceName}/providers/Microsoft.Bar/bars/{barName}", |
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.
the result changed because of the change above.
In this path, we have a default but it is not at the end.
Also I have concerns: why would we try to rebuild the resourceInstancePath from the path any way? Should we just use the READ request path as the resource instance path? I think ARM did this in their logic, and resource type is then built out of this path.
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.
do you think if this makes sense that I changed the result? Or we should just remove this?
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 created this issue to track this: #3620
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.
First, we need to target this PR at release/december-2025,
scond I have a couple of minor comments, and a slight algorithmic change:
- We should check for 'default' as well as the resourceInstance Name from the resource type
- We should check against the resourceInstanceName (and default) whwnever we are looking to see that an enven-numbered segment is a variable segment
I think this will give us better results in legacy scenarios
|
|
||
| function isVariableSegment(segment: string): boolean { | ||
| return (segment.startsWith("{") && segment.endsWith("}")) || segment === "default"; | ||
| function isVariableSegment(segment: string, singletonResourceName?: string): boolean { |
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
|
|
||
| if (i + 1 < segments.length && isVariableSegment(segments[i + 1])) { | ||
| // if the next segment is the last segment | ||
| if ( |
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 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
| expect(employee).toMatchObject({ | ||
| kind: "Tracked", | ||
| providerNamespace: "Microsoft.ContosoProviderHub", | ||
| type: expect.anything(), |
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.
we should check the resourceInstance path and the singleton names here as well
| expect(employee).toMatchObject({ | ||
| kind: "Tracked", | ||
| providerNamespace: "Microsoft.ContosoProviderHub", | ||
| type: expect.anything(), |
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.
we should check singleton resource names and resourceInstancePath
| providerNamespace: "Microsoft.ContosoProviderHub", | ||
| type: expect.anything(), | ||
| resourceType: { | ||
| provider: "Microsoft.ContosoProviderHub", |
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.
we should add in resourceInstancePath and singletonNames checks
956cd97 to
155ce30
Compare
Fixes #3595
Fixes #3623
Previously the segment default was hard-coded in a function checking if a segment in path is a variable.
This PR modifies it to accept an optional extra singletonResourceName parameter - if match, we will return true as variable segment.
This PR also updates the
ResolvedResourceAPI to add a new propertysingletonResourceNames