Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Disable false-positive flagging rules in PROD #643

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

AkhilaIlla
Copy link
Collaborator

@AkhilaIlla AkhilaIlla commented Dec 19, 2023

Folks have been complaining about rules being falsely flagged after the prod release.
This PR is to disable all those rules and also to fix RequestBodyMustExistForPutPatch(rules looks for the body param name to be "body" and it isn't mandatory for the body param's name to be "body", it is wrong and fixing it in this PR)

@@ -90,7 +90,7 @@ export function getProperty(schema: any, propName: string): any {
}

export function findBodyParam(params: any) {
const isBody = (elem: any) => elem.name === "body" && elem.in === "body"
const isBody = (elem: any) => elem.name === "body"
Copy link
Contributor

@tejaswiMinnu tejaswiMinnu Dec 19, 2023

Choose a reason for hiding this comment

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

What is this change for? is it auto generated? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, it isnt mandatory for the body param's nam eto be "body", it is wrong and fixing it in this PR

Copy link
Member

Choose a reason for hiding this comment

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

Oh, in that case shouldn't your change be:

Suggested change
const isBody = (elem: any) => elem.name === "body"
const isBody = (elem: any) => elem.in === "body"

since we don't care about the param name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, just fixed it!
thanks

Copy link
Contributor

@tejaswiMinnu tejaswiMinnu Dec 19, 2023

Choose a reason for hiding this comment

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

Any reason to change "body" to "testBody"? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just updating tests to check the fix I made

Copy link
Member

@bdefoy bdefoy Dec 19, 2023

Choose a reason for hiding this comment

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

Add another changelog file for the RequestBodyMustExistForPutPatch changes. You can run rush change again and there should be an option to add another change #Resolved

@@ -90,7 +90,7 @@ export function getProperty(schema: any, propName: string): any {
}

export function findBodyParam(params: any) {
const isBody = (elem: any) => elem.name === "body" && elem.in === "body"
const isBody = (elem: any) => elem.name === "body"
Copy link
Member

@bdefoy bdefoy Dec 19, 2023

Choose a reason for hiding this comment

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

Are we sure we don't need "in": "body" for body parameters? It looks like in OpenAPI 2.0 in is required for all parameters, but in OpenAPI 3.0 body parameters use a different property name: requestBody. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad,
The intent was to remove the name===body check

@@ -1,6 +1,6 @@
{
"name": "@microsoft.azure/openapi-validator-rulesets",
"version": "1.3.1",
"version": "1.3.2",
Copy link
Member

@bdefoy bdefoy Dec 19, 2023

Choose a reason for hiding this comment

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

When updating the version make sure to use rush version --bump so the changelog gets generated and the individual changelog files get deleted. See https://github.com/Azure/azure-openapi-validator/blob/main/CONTRIBUTING.md#create-a-release-pr #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point !
Ill do that, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

When you do version bump, you need to discard other file changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tejaswiMinnu wdym by other file changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that is expected.
when we do the version bump, individual changeLog files get deleted and all those are consolidated and added to the main changelog.json

@AkhilaIlla AkhilaIlla merged commit 1c1829e into main Dec 20, 2023
4 checks passed
@AkhilaIlla AkhilaIlla deleted the akhilailla/fix_linter_errors branch December 20, 2023 17:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants