-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Fix linter script to run all tags #3400
Conversation
Automation for azure-sdk-for-nodeUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-javaUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-goUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-rubyUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-pythonUnable to detect any generation context from this PR. |
@veronicagg @salameer FYI..... |
test/linter.js
Outdated
@@ -116,15 +122,21 @@ describe('AutoRest Linter validation:', function () { | |||
return prFile.startsWith(configDir) && prFile.indexOf('examples') === -1 && prFile.endsWith('.json'); | |||
}).forEach(prFileInConfigFile => { | |||
console.warn(`WARNING: Configuration file not found for file: ${prFileInConfigFile}, running validation rules against it in individual context.`); | |||
execLinterCommand(`--input-file=${prFileInConfigFile}`); | |||
errorsFound = errorsFound && execLinterCommand(`--input-file=${prFileInConfigFile}`); |
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.
execLinterCommand
will never execute since errorsFound
is always false right ?
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. But surprisingly it was executing all 3 tags during my testing. hmm....
Just to be on the safe side, I have rearranged the order.
test/linter.js
Outdated
@@ -102,6 +105,9 @@ describe('AutoRest Linter validation:', function () { | |||
|
|||
// find all tags in the config file | |||
const tagsToProcess = await getTagsFromConfig(config); | |||
|
|||
var errorsFound = false; |
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.
let
not var
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.
Thanks. Done
try { | ||
let result = execSync(cmd, { encoding: 'utf8', maxBuffer: 1024 * 1024 * 64 }); | ||
console.error(result); | ||
} catch (err) { | ||
throw new Error('Linter validation contains error(s)'); | ||
errorsFound = true; | ||
console.error('Linter validation contains error(s)'); |
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.
print the error also ?
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 Linter itself will print the error. So, printing it again is not required.
try { | ||
let result = execSync(cmd, { encoding: 'utf8', maxBuffer: 1024 * 1024 * 64 }); | ||
console.error(result); | ||
} catch (err) { | ||
throw new Error('Linter validation contains error(s)'); | ||
errorsFound = 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.
@sarangan12 I know we talked a bit offline, but could you provide details on the PR on the bug you're trying to fix? - like what issue you found and how it's not working with the current code.
@dsgouda I believe we worked on adding looping through tags a while back and can't remember why we decided to exit early, do you?
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.
Created an issue and provided a detailed explanation and also linked the issue with this PR
test/linter.js
Outdated
@@ -116,15 +122,21 @@ describe('AutoRest Linter validation:', function () { | |||
return prFile.startsWith(configDir) && prFile.indexOf('examples') === -1 && prFile.endsWith('.json'); | |||
}).forEach(prFileInConfigFile => { | |||
console.warn(`WARNING: Configuration file not found for file: ${prFileInConfigFile}, running validation rules against it in individual context.`); | |||
execLinterCommand(`--input-file=${prFileInConfigFile}`); | |||
errorsFound = errorsFound && execLinterCommand(`--input-file=${prFileInConfigFile}`); |
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.
should this be an OR statement? same below
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.
well with OR we would just have the old behavior ( once it founds errors it would stop processing further ). I think the intention of the processing to continue. In that case just execute the command seperatley and && on the result
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.
Actually, if you put ||, it will still continue executing all the tags. But, the job itself will be marked as success irrespective of presence of the errors. We do not want to do that. That is the reason for &&.
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.
why will it continue ? if we put ||
once errorsFound becomes true
( i.e. one of them fails) then it will not execute execLinterCommand
because of the shortcircuit logic right ? ( for files here for tags afterwards )
@@ -82,13 +82,16 @@ async function getTagsFromConfig(config) { | |||
function execLinterCommand(args) { | |||
var cmd = `npx autorest@2.0.4152 --validation --azure-validator --message-format=json ${args}`.trim(); |
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 just use autorest as a library and call the linter through it
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 use autorest as a library in the tooling service you can have a look there how to do it, that way also we wont have the autorest version hardcoded in the 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.
Sure. I will look into it and take it up as a seperate issue.
I have no background or information about linter. Reassigning to @veronicagg |
Addressed the comments. Merging it now |
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger