-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
console.log(`Executing: ${cmd}`); | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
console.error('Linter validation contains error(s)'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
} | ||
|
||
return errorsFound; | ||
} | ||
|
||
describe('AutoRest Linter validation:', function () { | ||
|
@@ -102,6 +105,9 @@ describe('AutoRest Linter validation:', function () { | |
|
||
// find all tags in the config file | ||
const tagsToProcess = await getTagsFromConfig(config); | ||
|
||
let errorsFound = false; | ||
|
||
// if no tags found to process, run with the defaults | ||
if (tagsToProcess === null || tagsToProcess.length === 0) { | ||
// no tags found | ||
|
@@ -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 = execLinterCommand(`--input-file=${prFileInConfigFile}`) && errorsFound; | ||
}); | ||
} | ||
else { | ||
// if tags found, run linter against every single tag | ||
tagsToProcess.forEach((tagToProcess) => { | ||
execLinterCommand(`${config} --tag=${tagToProcess}`); | ||
errorsFound = execLinterCommand(`${config} --tag=${tagToProcess}`) && errorsFound; | ||
}, this); | ||
} | ||
|
||
if(errorsFound == true) { | ||
throw new 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.
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.