Running linter with all tags in .md files #1491

dsgouda · 2017-07-28T22:26:19Z

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

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

veronicagg

looks good, left one minor question inline.
Have you run it on the repo, any idea on how much more time this may take?

veronicagg · 2017-07-28T22:37:35Z

test/linter.js

    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();


are you trimming to remove the space if no tags are passed?

veronicagg · 2017-07-31T18:57:54Z

should we also filter the output to the files that are included in each submitted PR, so people can more easily read it?

veronicagg · 2017-07-31T18:59:36Z

Referencing issue this is addressing: #1424

dsgouda · 2017-07-31T19:58:59Z

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.
Something to think about

… into cifix

veronicagg · 2017-08-03T21:04:14Z

test/linter.js

-  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)}`);


do we need to call the app.js file explicitly?
should we have a require for this package at the top of the file?

it'd be nice to write a comment with an example of what result looks like (the output of the tool)

veronicagg · 2017-08-03T22:02:44Z

test/linter.js

+    // if no tags found to process, run with the defaults    
+    if(tagsToProcess===undefined || tagsToProcess.length===0)
+    {
+      execLinterCommand(config, '');


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?

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)

olydis · 2017-08-03T22:49:55Z

test/linter.js

+  const tags = Object.keys(tagsMap);
+
+  // filter the tags
+  if (utils.prOnly) {


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?

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.

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! 😆

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

olydis · 2017-08-03T22:51:05Z

test/linter.js

+    // config files have relative paths to the input files
+    let allModifiedFiles = utils.getFilesChangedInPR();
+    allModifiedFiles = allModifiedFiles.map(mfile => {
+      return mfile.replace(path.dirname + '/', '');


did you test this? path.dirname is a function AFAIK, not sure what this is supposed to do

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.

I made that dirname change post testing 🙁

olydis · 2017-08-03T23:00:37Z

test/linter.js

+
+    // for each tag->files, find if there are any modified files and select those tags
+    return tags.filter(tag => {
+      const tagFiles = (tagsMap[tag] + '').split(',');


voodoo... rather ensure the data type at creation time of tagsMap 😉

How do you define types at declaration time in javascript? 😕

olydis · 2017-08-03T23:06:16Z

test/linter.js

+        // 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();


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!

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?

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.

Sounds good, I can add a console.warn here.

@dsgouda @veronicagg Ah, did not think of that, makes perfect sense then! +1 on the warning.

veronicagg · 2017-08-04T00:53:46Z

test/linter.js

+  async_io_1 = require("@microsoft.azure/async-io"),
+  path = require('path');
+
+async function readConfigFile(file, tag) {


are the machines that will run this running new versions of JS? that support async/await?

AFAIK yes, @olydis please confirm

veronicagg · 2017-08-04T00:55:50Z

test/linter.js

+  return result;
+}
+
+function scanForTags(content) {


do we need to scan for tags? is the literate module not providing us with a list?

This is a function I copied over from the literate module itself (the function is not exported publicly)

veronicagg · 2017-08-04T01:02:29Z

test/linter.js

+        // 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();


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.

olydis · 2017-08-04T18:07:27Z

test/linter.js

+          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}`, '');


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.

AutorestCI · 2017-08-04T21:03:24Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-08-04T21:03:57Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-08-04T21:04:02Z

No modification for AutorestCI/azure-sdk-for-node

This reverts commit 7ef91d6.

AutorestCI · 2017-08-04T23:01:53Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-08-04T23:02:05Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-08-04T23:02:10Z

No modification for AutorestCI/azure-sdk-for-ruby

…" (Azure#1509)" This reverts commit 27b7a74.

* Revert "Running linter with all tags in .md files (#1491)" This reverts commit 7ef91d6. * Revert "Revert "Running linter with all tags in .md files (#1491)" (#1509)" This reverts commit 27b7a74.

dsgouda requested review from olydis and veronicagg July 28, 2017 22:26

msftclas added the cla-not-required label Jul 28, 2017

azuresdkci assigned anuchandy Jul 28, 2017

veronicagg reviewed Jul 28, 2017

View reviewed changes

Running linter with all tags in .md files

50a2494

dsgouda force-pushed the cifix branch from bbe531b to 50a2494 Compare August 3, 2017 18:54

dsgouda added 3 commits August 3, 2017 11:57

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

620171a

… into cifix

coding goodness

b9cba4c

code awesomeness

fb4144c

veronicagg suggested changes Aug 3, 2017

View reviewed changes

dsgouda added 3 commits August 3, 2017 15:08

cleanup

3083b32

when no tags are found

286891b

cleanup

64f8cfd

olydis suggested changes Aug 3, 2017

View reviewed changes

Addressed PR comments

a00b0f4

veronicagg suggested changes Aug 4, 2017

View reviewed changes

more PR comments

7ac6c50

veronicagg approved these changes Aug 4, 2017

View reviewed changes

olydis reviewed Aug 4, 2017

View reviewed changes

More PR comments

293ceb1

olydis approved these changes Aug 4, 2017

View reviewed changes

dsgouda merged commit 7ef91d6 into Azure:current Aug 4, 2017

dsgouda deleted the cifix branch August 4, 2017 21:00

dsgouda added a commit to dsgouda/azure-rest-api-specs that referenced this pull request Aug 4, 2017

Revert "Running linter with all tags in .md files (Azure#1491)"

a71d2cf

This reverts commit 7ef91d6.

olydis pushed a commit that referenced this pull request Aug 4, 2017

Revert "Running linter with all tags in .md files (#1491)" (#1509)

27b7a74

This reverts commit 7ef91d6.

dsgouda added a commit to dsgouda/azure-rest-api-specs that referenced this pull request Aug 7, 2017

Revert "Revert "Running linter with all tags in .md files (Azure#1491)…

1b4b98a

…" (Azure#1509)" This reverts commit 27b7a74.

Running linter with all tags in .md files #1491

Running linter with all tags in .md files #1491

Conversation

dsgouda commented Jul 28, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg commented Jul 31, 2017

veronicagg commented Jul 31, 2017

dsgouda commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgouda Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsgouda Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olydis Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

AutorestCI commented Aug 4, 2017

AutorestCI commented Aug 4, 2017

AutorestCI commented Aug 4, 2017

AutorestCI commented Aug 4, 2017

AutorestCI commented Aug 4, 2017

AutorestCI commented Aug 4, 2017

dsgouda commented Jul 28, 2017 •

edited by azuresdkci

Loading

dsgouda Aug 3, 2017 •

edited

Loading

dsgouda Aug 4, 2017 •

edited

Loading

olydis Aug 4, 2017 •

edited

Loading