Fix TopLevelResourcesListBySubscription to exclude tenant level API's…#808
Fix TopLevelResourcesListBySubscription to exclude tenant level API's…#808AkhilaIlla wants to merge 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the TopLevelResourcesListBySubscription ARM rule that caused validation to stop as soon as any tenant-level API path was encountered, and adds tests to ensure correct behavior for tenant-only and mixed-scope specs.
Changes:
- Updated
TopLevelResourcesListBySubscriptionto:- Use full
ResourceInfoobjects instead of just names. - Skip validation when a spec has no subscription-scoped paths at all (including
x-ms-paths). - Only validate top-level resources that actually have subscription-scoped operations.
- De-duplicate validation per model name and ignore purely tenant-scoped resources.
- Use full
- Extended
composite-azure-testswith new test cases for:- Tenant-only specs.
- Mixed tenant and subscription specs missing list-by-subscription.
- Subscription-scoped paths defined under
x-ms-paths.
- Added three focused Swagger fixtures to model tenant-only, mixed tenant/subscription, and
x-ms-pathssubscription scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/rulesets/src/native/legacyRules/TopLevelResourcesListBySubscription.ts |
Refines rule logic to correctly distinguish tenant-only specs, require list-by-subscription only for resources with subscription-scoped operations, and avoid duplicate reports per model. |
packages/rulesets/src/native/tests/composite-azure-tests.ts |
Adds targeted tests for tenant-only, mixed-scope, and x-ms-paths subscription scenarios and clarifies the existing compute test comment. |
packages/rulesets/src/native/tests/resources/armResource/topLevelListBySubscription_tenantOnlyResource.json |
New tenant-only Swagger fixture to verify that list-by-subscription validation is skipped when no subscription-scoped paths exist. |
packages/rulesets/src/native/tests/resources/armResource/topLevelListBySubscription_subscriptionInXmsPaths_missingCollection.json |
New Swagger fixture to ensure subscription-scoped paths under x-ms-paths still trigger list-by-subscription validation. |
packages/rulesets/src/native/tests/resources/armResource/topLevelListBySubscription_mixedTenantAndSubscription_missingCollection.json |
New Swagger fixture to confirm that mixed tenant and subscription specs still enforce list-by-subscription for subscription-scoped top-level resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| // Skip validation if there is no subscription-scoped paths in the spec |
There was a problem hiding this comment.
Somehow this block causes the rule to terminate entirely when a spec contains ANY tenant-level API,
even if the spec also has subscription-scoped resources that need validation.
Subscription-scoped resources should be validated for list operations per resources that you are already handling on lines 39 - 42
There was a problem hiding this comment.
I dont understand.
line 23 checks for all the sub level paths in the spec.
line 24 is for early termination if there arent any sub paths.
I dont think this actually skips validating the sub paths., right?
There was a problem hiding this comment.
You're right that line 23 checks for subscription-level paths and line 24 exits if there aren't any.
But that's exactly the problem, the rule's job is to detect MISSING list-by-subscription operations.
If the list-by-subscription operation is missing (which is what we want to flag),
then hasAnySubscriptionScopedPath will be false, and line 24 exits early.
We can't report missing subscription-scoped paths if we exit when they're missing
My suggestion is to remove lines 21-26
There was a problem hiding this comment.
Thats a very good point. I'm double checking into if we can ever end up in that scenario.
Checking what top-level-resources actually are.
|
@AkhilaIlla We need to update package.json |
|
@mikeharder I've opened a new pull request, #814, to work on those changes. Once the pull request is ready, I'll request review from you. |
@copilot: No, add the changes to this PR. |
|
@mikeharder I've opened a new pull request, #815, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ription fix (#815) * Initial plan * Update package.json and CHANGELOG.md for version 2.2.1 Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com>
|
@copilot: increment the patch version by one |
|
@mikeharder I've opened a new pull request, #818, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Increment patch version from 2.2.1 to 2.2.2 Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mikeharder <9459391+mikeharder@users.noreply.github.com>
The fix in the PR - Fix TopLevelResourcesListBySubscription to author tenant-level APIs by tejaswiMinnu · Pull Request … introduced a regression which makes the rule terminate even if a single tenant level API is encountered/present in the spec file.
The spec in this Typespec for Postgres Migration RP by nikhilhope3 · Pull Request #25493 · Azure/azure-rest-api-specs-pr has-
"/providers/Microsoft.DataMigration/operations"
making the rule to terminate and not validate further.
This PR addresses-https://msazure.visualstudio.com/One/_workitems/edit/36410442 and fixes the regression and also addresses the scenario to skip tenant level API's from validation.