-
Notifications
You must be signed in to change notification settings - Fork 46
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
Update PatchPropertiesCorrespondToPutProperties to run in stagingOnly #596
Update PatchPropertiesCorrespondToPutProperties to run in stagingOnly #596
Conversation
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.
@AkhilaIlla I think you should bump the ruleset package version. I am not blocking the PR as I will be away for few hours now, but please don't merge until you bump the package version.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@microsoft.azure/openapi-validator", | |||
"version": "2.1.5", | |||
"version": "2.1.6", |
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.
This shouldn't be changed in this PR: the only changes are in packages/rulesets
dir.
@AkhilaIlla Observe the dir layout:
And the names of the packages in the dirs:
You can also explore information about the published package versions on npm, e.g.:
https://www.npmjs.com/package/@microsoft.azure/openapi-validator-rulesets
This includes package dependencies. It helps to build a mental model.
Can you update the contributing guide to make the instructions clearer? Please add me as a reviewer.
#Resolved
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.
Yup lets improve the doc so even first timers can cruise thru this.
@@ -64,7 +64,7 @@ | |||
"@stoplight/types": "^13.5.0", | |||
"dependency-graph": "^0.11.0", | |||
"@microsoft.azure/openapi-validator-core": "~1.0.0", | |||
"@microsoft.azure/openapi-validator-rulesets": "^1.1.1", | |||
"@microsoft.azure/openapi-validator-rulesets": "^1.1.2", |
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.
This won't hurt, but strictly speaking there is no need for that, because of ^
. https://semver.npmjs.com/
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.
You mean there isnt any necessity for ^ here right?
Also the latest version is 1.2.0 and now will be 1.2.1 and should this still be 1.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.
Observe the version is ^1.1.1
. Which means it will accept anything of form 1.X.Y
where X >= 1
and Y >= 1
. So given you are changing ruleset from 1.2.0
to 1.2.1
, it will pick it up. Also observe you are bumping it here from 1.1.1
to 1.1.2
but you changed the ruleset from 1.2.0
to 1.2.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.
that makes sense. thanks so much!
@@ -0,0 +1,10 @@ | |||
{ |
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.
This file change shouldn't be in this PR. #Resolved
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.
Builds fail without this file
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.
It fails because there are changes to azure-openapi-validator/autorest/package.json
. If you revert them (please do that) then it will pass.
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.
🕐
@@ -3371,6 +3372,7 @@ const ruleset = { | |||
description: "Extension resources are always considered to be proxy and must not be of the type tracked.", | |||
message: "{{error}}", | |||
severity: "error", | |||
stagingOnly: true, |
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 am curious, is this some leftover from previous change that was not generated into the .js
file from .ts
file? (that is, someone forgot to run all the steps from rush prep
from step 4. here). Also, btw. rule name is lower-cased: trackedExtensionResourcesAreNotAllowed
. Should be upper-cased. But we can fix it in another PR.
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.
Left one non-blocking comment. Also would be nice to add some context in the PR description. Besides, looks good. Thx!
I verified the PR is good to go.
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.
@AkhilaIlla now that the PR is merged, please follow the instructions here: I think at some point you will have to ping me to approve your prod release:
|
The LintDiff pipeline is failing with
https://teams.microsoft.com/l/message/19:[email protected]/1695277014925?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=0cab4ce9-7691-42ae-82e3-460d4346a710&parentMessageId=1692914350538&teamName=ARM%20API%20Reviewers&channelName=ARM%20linter%20rules&createdTime=1695277014925
and hence the faulty rule is updated to run only in staging