-
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
Running linter with all tags in .md files #1491
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.
looks good, left one minor question inline.
Have you run it on the repo, any idea on how much more time this may take?
test/linter.js
Outdated
it(config + ' should honor linter validation rules.', async function () { | ||
var cmd = `autorest --validation --azure-validator ${config} --message-format=json`; | ||
var cmd = `autorest --validation --azure-validator ${config} --message-format=json ${tag}`.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.
are you trimming to remove the space if no tags are passed?
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!
should we also filter the output to the files that are included in each submitted PR, so people can more easily read it? |
Referencing issue this is addressing: #1424 |
Could be risky, if they are referencing a model/property from some other file which in turn has issues, linter would report that file and we might miss out on those errors. |
test/linter.js
Outdated
for (const config of configsToProcess) { | ||
function getTagsFromConfig(config){ | ||
// get hold of all tags and their corresponding input files using the literate config tool | ||
const result = execSync(`node node_modules/@microsoft.azure/literate/dist/app.js all --file=${path.resolve(config)}`); |
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.
do we need to call the app.js file explicitly?
should we have a require for this package at the top of 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.
it'd be nice to write a comment with an example of what result looks like (the output of the tool)
test/linter.js
Outdated
// if no tags found to process, run with the defaults | ||
if(tagsToProcess===undefined || tagsToProcess.length===0) | ||
{ | ||
execLinterCommand(config, ''); |
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.
if tags are not found, running the linter with the readme file won't provide any benefit, as far as I understand. In the case tags are not found, I think we need to execute autorest --input-file= --azure-validator --message-format=json
thoughts?
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.
Couldn't there be cases without a tag but with input file sections in the configuration files (I am open to this solution but this is a valid case too)
const tags = Object.keys(tagsMap); | ||
|
||
// filter the tags | ||
if (utils.prOnly) { |
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.
what's this flag supposed to do? note that even if this is false
, all you process are the getConfigFilesChangedInPR
. Is this intended, i.e. what's the purpose of checking all tags vs. only affected ones?
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.
Checking only affected tags means we will be running the validator for contexts affected by the files changed in current PR and no other contexts, idea is to reduce noise as much as possible.
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 sure I got that, but it seems inconsistent (correct me if I'm wrong): Further down I see let configsToProcess = utils.getConfigFilesChangedInPR();
which is not guarded by this flag... so we always only look at affected config files, but then run on all tags? Ahhh, or does getConfigFilesChangedInPR
itself return all files if !prOnly
? If so, its name is very misleading! 😆
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.
Per offline discussion with @vishrutshah we currently don't have a way to run the linter in the PR_only=false
mode, we may have to write some code to get the list of all md files in the repo for that (trivial but not high priority as of now)
If we do write that code getTagsFromConfig
can handle both cases, I prefer we keep this as is but certainly fix let configsToProcess = utils.getConfigFilesChangedInPR();
@veronicagg do let us know your thoughts here
test/linter.js
Outdated
// config files have relative paths to the input files | ||
let allModifiedFiles = utils.getFilesChangedInPR(); | ||
allModifiedFiles = allModifiedFiles.map(mfile => { | ||
return mfile.replace(path.dirname + '/', ''); |
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.
did you test this? path.dirname
is a function AFAIK, not sure what this is supposed to do
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.
Guess: you wanna get the file name without directory - think there's a function for that, so use that instead of doing string magic
Result: your tag filtering won't work correctly, since the directories are where the filtering can happen! most tags differ only by the API version of files they include so the directory is crucial. So currently, I think you would run the linter against all tags either way.
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 made that dirname
change post testing 🙁
test/linter.js
Outdated
|
||
// for each tag->files, find if there are any modified files and select those tags | ||
return tags.filter(tag => { | ||
const tagFiles = (tagsMap[tag] + '').split(','); |
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.
voodoo... rather ensure the data type at creation time of tagsMap
😉
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.
How do you define types
at declaration time in javascript? 😕
test/linter.js
Outdated
// this means we need to run validator against the individual | ||
// json files included in the PR | ||
// but in the same directory tree as the config file | ||
const filesChangedInPR = utils.getFilesChangedInPR(); |
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.
not sure about this - this case indicates a huge data quality problem in the repo. Sure, maybe do some validation even in that case, but maybe just throwing would be better so the PR that creates the issue is rejected!
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.
In theory yes, but we have seen cases where teams like to add jsons without referencing them in the composite jsons (or readmes now), won't be a good idea to disable validation in such cases.
Having said that, I am open to the idea of blocking on it too,
@veronicagg thoughts?
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 would prefer the validation on the files.
How about printing a message that says, "WARNING: We couldn't find your file(s) included in the configuration file " along with validation?
We may want to have some consistency checks on the readme and know if there are json files not included anywhere, but I would keep the current linter validation to what we know, at the moment.
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.
Sounds good, I can add a console.warn here.
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.
@dsgouda @veronicagg Ah, did not think of that, makes perfect sense then! +1 on the warning.
test/linter.js
Outdated
async_io_1 = require("@microsoft.azure/async-io"), | ||
path = require('path'); | ||
|
||
async function readConfigFile(file, tag) { |
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.
are the machines that will run this running new versions of JS? that support async/await?
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.
AFAIK yes, @olydis please confirm
return result; | ||
} | ||
|
||
function scanForTags(content) { |
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.
do we need to scan for tags? is the literate module not providing us with a list?
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 is a function I copied over from the literate module itself (the function is not exported publicly)
test/linter.js
Outdated
// this means we need to run validator against the individual | ||
// json files included in the PR | ||
// but in the same directory tree as the config file | ||
const filesChangedInPR = utils.getFilesChangedInPR(); |
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 would prefer the validation on the files.
How about printing a message that says, "WARNING: We couldn't find your file(s) included in the configuration file " along with validation?
We may want to have some consistency checks on the readme and know if there are json files not included anywhere, but I would keep the current linter validation to what we know, at the moment.
test/linter.js
Outdated
return prFile.startsWith(configDir) && prfile.indexOf('examples') === -1 && prFile.endsWith('.json'); | ||
}).forEach(prFileInConfigFile => { | ||
console.warn(`Configuration file not found for file: ${prFileInConfigFile}, running validation rules against it in individual context.`); | ||
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.
Since execLinterCommand
does nothing smart about its two parameters (they're just interpolated into the command to run... you could pass "--csharp"
as tags
), I think the signature should be execLinterCommand(...additionalArgs: string[])
, then you can call it as
execLinterCommand(`--input-file=${prFileInConfigFile}`)
More readable and there's no parameter names that suggest semantics that don't exist.
No modification for AutorestCI/azure-sdk-for-ruby |
No modification for AutorestCI/azure-sdk-for-python |
No modification for AutorestCI/azure-sdk-for-node |
This reverts commit 7ef91d6.
No modification for AutorestCI/azure-sdk-for-python |
No modification for AutorestCI/azure-sdk-for-node |
No modification for AutorestCI/azure-sdk-for-ruby |
…" (Azure#1509)" This reverts commit 27b7a74.
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.
@olydis any ideas on how we can test this?
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger