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.
execLinterCommand will never execute since errorsFound is always false right ?
There was a problem hiding this comment.
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; | |||
| 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.
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.
@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.
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.
should this be an OR statement? same below
There was a problem hiding this comment.
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.
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.
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.
we should just use autorest as a library and call the linter through it
There was a problem hiding this comment.
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.
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-versionin the path should match theapi-versionin the spec).Quality of Swagger