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.

Sign up for GitHub

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

Jump to bottom

[AvoidAdditionalProperties] Set resolved:false to only report errors at the source #700

Merged
merged 5 commits into from
Jun 3, 2024
Merged

[AvoidAdditionalProperties] Set resolved:false to only report errors at the source #700

merged 5 commits into from
Jun 3, 2024

Conversation

mikeharder
Copy link
Member

@mikeharder mikeharder commented Jun 1, 2024 •
edited
Loading

Verified it fixes the full spec in a local end-to-end test:

$ npx autorest --v3 --spectral --azure-validator --semantic-validator=false --model-validator=false \
  --openapi-type=arm --openapi-subtype=arm [email protected]/[email protected] \
  --tag=package-preview-2024-04 specification/desktopvirtualization/resource-manager/readme.md \
 | grep -i avoidadditionalproperties

error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.
error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.
error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.
error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.
error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.
error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.

$ npx autorest --v3 --spectral --azure-validator --semantic-validator=false --model-validator=false \
  --openapi-type=arm --openapi-subtype=arm --use=./packages/azure-openapi-validator/autorest \
  --tag=package-preview-2024-04 ../specs/specification/desktopvirtualization/resource-manager/readme.md \
  | grep AvoidAdditionalProperties

error   | AvoidAdditionalProperties | Definitions must not have properties named additionalProperties except for user defined tags or predefined references.

@mikeharder mikeharder requested a review from markcowl June 1, 2024 05:46
@mikeharder mikeharder marked this pull request as ready for review June 1, 2024 06:01
@mikeharder mikeharder changed the title [AvoidAdditionalProperties] Add test for error via $ref [AvoidAdditionalProperties] Set resolved:false to only report errors at the source Jun 1, 2024
@mikeharder mikeharder self-assigned this Jun 1, 2024
konrad-jamrozik
konrad-jamrozik reviewed Jun 3, 2024
View reviewed changes
packages/rulesets/src/spectral/az-arm.ts
@@ -299,7 +299,8 @@ const ruleset: any = {
message: "{{description}}",
disableForTypeSpec: true,
disableForTypeSpecReason: "Covered by TSP's '@azure-tools/typespec-azure-resource-manager/no-record' rule.",
resolved: true,
// Only report errors at the source, not from inside $refs (the resolved document)
resolved: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I believe the resolved is defined here:
https://docs.stoplight.io/docs/spectral/d3482ff0ccae9-rules#resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. After reading that, I feel even more confident this rule should use resolved: false.

@mikeharder mikeharder merged commit 9e4296a into Azure:main Jun 3, 2024
4 checks passed
@mikeharder mikeharder deleted the avoid-additional-properties-refs branch June 3, 2024 19:27
@mikeharder mikeharder mentioned this pull request Jun 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konrad-jamrozik konrad-jamrozik konrad-jamrozik approved these changes

@markcowl markcowl Awaiting requested review from markcowl

@rkmanda rkmanda Awaiting requested review from rkmanda rkmanda is a code owner

@AkhilaIlla AkhilaIlla Awaiting requested review from AkhilaIlla AkhilaIlla is a code owner

Assignees

@mikeharder mikeharder

Labels
None yet
Projects
Azure SDK EngSys 📈
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AvoidAdditionalProperties] Errors should only be reported once per source code location (ignore $refs)
2 participants
@mikeharder @konrad-jamrozik