Add live validation by vladbarosan · Pull Request #2211 · Azure/azure-rest-api-specs

vladbarosan · 2018-01-04T02:27:24Z

Add job for live validation of swagger changes.

vladbarosan · 2018-01-04T07:27:33Z

@veronicagg the python and ruby builds are failing. For some reason Ruby tries to build specification/compute/resource-manager/Microsoft.Compute/2017-03-30 while Python tries to build cognitiveservices.vision.customvision.prediction which don't exit.

Looking at another build job it also would try there, but it skips since no file is involved in the PR. In this case it seems to build everything ( because no swagger is changed ? ).

Do you know where that list is coming from and can we update it ?

veronicagg · 2018-01-04T18:35:53Z

@vladbarosan with the latest reorganization of the repo (preview/stable) the specs are not were they used to be anymore, so our https://github.com/Azure/azure-sdk-for-ruby/blob/master/swagger_to_sdk_config.json is out of date for the cases where we point to specific specs (not readme.md files). I'll make a PR against azure-sdk-for-ruby to update the swagger_to_sdk_config.json file with the new paths. Thanks!
For the Python issue, @lmazuel should be able to help.

lmazuel · 2018-01-04T23:58:27Z

@vladbarosan Python is failing because I already updated swagger-to-sdk to generate the content of this unmerged PR:
#2177

Since Tuesday, SwaggerToSdk is able to configure itself automatically by parsing the Readme looking for a "swagger-to-sdk" section. Once I migrated the content from swagger-to-sdk conf, this kind of problem will no longer happen. In progress, sorry for the disagreement in the meantime.

vladbarosan · 2018-01-05T00:56:54Z

@veronicagg Sample output on validation failure https://travis-ci.org/vladbarosan/azure-rest-api-specs/jobs/325251898

At point preferably to have the result and the link to the logs posted to the PR.

veronicagg

Comments inline.

veronicagg · 2018-01-05T00:55:26Z

.travis.yml

+    - MODE=liveValidation PR_ONLY=true
  global:
-    secure: n7Ptb5x7Zdq/7na2L68JlrRPbl/xWtiFQncO7fsrSVuVGI4Mhjj1LD2k07qWAdFPM1PaZYvCRWc3YwbXr1NTOOn485r6qcLUjpN9TICP0ErGcEA9SzctipfbFP1IV4aCh23WSaopMueBki52KfskaGcZ2ox9pvI2LCysCp7q4WwF/0vArLYf48FeZuscWHVaLsU4z8ZMFPT9yHg+RQoqPeBnaR/ZGtG96NZolT+VAlP445geb0qn8wWAuSl4bR1JQV2eA3MwdWu/iVkepBeKTN7d81l3yjWzTvFtP/JRJClWBNQbOMjAed3/2Tr2lGgyRUM6Uwp4KvfRmbX3Xrlggen1AO/YAb2mJDs+HARPnhZXIOtDjgft/ethVeLTTBUsbNFGUN2lcrJMw9dkFi+ai3XefatENJbULqWx8Xb+wMD1b0TrI6sZZpdC8vYHnM/DoqiEh5+h3okkSmQcjPF4K9js294G790PKf4u2CQdur41qh4Ze4IzbvulVsGMON+O93vA6ZRK1sTHC5VUTs2iMhCnf6LtMN59kBq1T2MJ8ndXRpGlFEn2wDTjJSpSr0k0sjoG5i+bIFhbqVF8xegBb/PH4H9/i1ifIypNNp0FGdz4o2cFsYrcUKFsx1C/kFJ6rC809r0odDt32hGSOkOCziTwFHusbR5Nwf0RjOWSgvc=
+    secure: >-


what's the reason for this change?

Automatic on save

In reply to: 159791961 [](ancestors = 159791961)

veronicagg · 2018-01-05T00:56:49Z

.travis.yml

    - env: MODE=model PR_ONLY=true
    - env: MODE=BreakingChange PR_ONLY=true
    - env: MODE=azurebot PR_ONLY=true
+    - env: MODE=liveValidation PR_ONLY=true


should we remove "env: MODE=linter PR_ONLY=false" since you're cleaning up the file anyway?

yes if we don't need it

In reply to: 159792088 [](ancestors = 159792088)

veronicagg · 2018-01-05T00:57:39Z

.travis.yml

-  - python -c "import os; print('\n'.join(v for v in os.environ.keys() if v.startswith('TRAVIS')))" > /tmp/env_file
-  # Required for installing dotnet 2.0.0 according to https://www.microsoft.com/net/core#linuxubuntu
-  - curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor > microsoft.gpg
+  - >-


does ">-" just allow multiple lines on the script?

yes.

In reply to: 159792157 [](ancestors = 159792157)

veronicagg · 2018-01-05T00:58:13Z

.travis.yml

 before_install:
  - docker pull lmazuel/swagger-to-sdk
-  - python -c "import os; print('\n'.join(v for v in os.environ.keys() if v.startswith('TRAVIS')))" > /tmp/env_file
-  # Required for installing dotnet 2.0.0 according to https://www.microsoft.com/net/core#linuxubuntu


why are we removing this comment?

I have the format on save, which did most of these changes :)

In reply to: 159792221 [](ancestors = 159792221)

veronicagg · 2018-01-05T01:00:14Z

.travis.yml

+  - >-
+    if [[ $MODE == 'ruby' ]]; then $DOCKER_CMD AutorestCI/azure-sdk-for-ruby
+    --pr-repo-id Azure/azure-sdk-for-ruby -o master -v; fi
+  - 'if [[ $MODE == ''syntax'' ]]; then npm test -- test/syntax.js; fi'


why do we need quotes around these lines? and double quotes for the MODE values?

veronicagg · 2018-01-05T01:01:42Z

package.json

+    "@microsoft.azure/async-io": "^1.0.21",
+    "@microsoft.azure/literate": "^1.0.21",
+    "@microsoft.azure/polyfill": "^1.0.17",
+    "azure-storage": "^2.1.0",


what do you need azure-storage dependency for?

it was already here, just sorted in alphabetic order. not sure who has dependency on it, searching in the repo there is no reference, I can remove it

In reply to: 159792564 [](ancestors = 159792564)

veronicagg · 2018-01-05T01:02:56Z

scripts/liveValidation.js

+    validationService = "https://app.azure-devex-tools.com/api/validations",
+    branch = utils.getSourceBranch(),
+    processingDelay = 20,
+    isRunningInTraviCI = process.env.MODE === 'liveValidation' && process.env.PR_ONLY === 'true',


typo in name, "Travi" -> "Travis"

veronicagg · 2018-01-05T01:11:19Z

scripts/liveValidation.js

+        return;
+    }
+
+    let resourceProvider = validationModels.keys().next().value;


could this fail? is there always a keys().next().value?

are you only expecting 1 resourceProvider?

same as for api version

In reply to: 159793672 [](ancestors = 159793672)

here it won't fail since we just return before that if the validationModels is empty (i.e. there is no swagger in the change)

veronicagg · 2018-01-05T01:11:39Z

scripts/liveValidation.js

+    }
+
+    let resourceProvider = validationModels.keys().next().value;
+    let apiVersion = validationModels.get(resourceProvider).values().next().value;


similar question to the one above for this

are you only expecting 1 api version, what if there are multiple involved in the changed files?

yes currently only 1 api version supported so getting only first 1. Will actually put a message in case tehre are multiple to inform that only the first match gets used

In reply to: 159793778 [](ancestors = 159793778)

ok, regarding multiple resource providers or multiple api-versions. It's good if there's a clear message about that. You may want to log an issue in your tool repo about this, so you can refer to it when it occurs in a PR, I'm sure it'll be occurring and you can collect feedback and number of instances by referring to it.

veronicagg · 2018-01-05T01:48:15Z

scripts/liveValidation.js

+    let validationId = JSON.parse(response).validationId;
+
+    let validationResultUrl = `${validationService}/${validationId}`;
+    console.log(`Request done, results will be available at ${validationResultUrl} in ${durationInSeconds} seconds...`);


looking at the sample output, not sure that the link in this line is that helpful, it does contain the result, but not easily readable, so you may want to include this link in a "log" line that doesn't lead the user to use it. The way it's included here, seems like the first thing I'd check.

That is there just for reference, after the result is returned it displays the results. Take a look at https://travis-ci.org/vladbarosan/azure-rest-api-specs/jobs/325251898 for a sample

You might be right to hide it though as it might distract, will update.

veronicagg · 2018-01-09T01:49:08Z

.travis.yml

This is just coming back as commented out, as it had been removed in the previous commit. thanks!

veronicagg · 2018-01-09T01:50:58Z

scripts/liveValidation.js

minor typo: upper case "S" isRunningInTraviSCI

veronicagg · 2018-01-09T01:52:49Z

scripts/liveValidation.js

"will be ready in" ?
I think it's good to remove it from here, if the output is useful for debugging (particularly for you or reviewers) feel free to write out the link, just somewhere else maybe in parenthesis or something - like "Note: raw results at "

well actually I print later everything that comes from there, but maybe ill add it as you mentioned "with raw results at"

veronicagg · 2018-01-09T01:55:56Z

scripts/liveValidation.js

+    }
+
+    let resourceProvider = validationModels.keys().next().value;
+    let apiVersion = validationModels.get(resourceProvider).values().next().value;


ok, regarding multiple resource providers or multiple api-versions. It's good if there's a clear message about that. You may want to log an issue in your tool repo about this, so you can refer to it when it occurs in a PR, I'm sure it'll be occurring and you can collect feedback and number of instances by referring to it.

veronicagg

few minor comments, approving PR, thanks!

AutorestCI · 2018-01-09T18:32:25Z

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

AutorestCI · 2018-01-09T18:36:58Z

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

vladbarosan added the Tooling Team label Jan 4, 2018

vladbarosan self-assigned this Jan 4, 2018

vladbarosan requested a review from veronicagg January 4, 2018 02:27

veronicagg mentioned this pull request Jan 4, 2018

Swagger to sdk updates due to azure-rest-api-specs repo reorg Azure/azure-sdk-for-ruby#1216

Merged

vladbarosan force-pushed the addLiveValidation branch from ec72d91 to 0638339 Compare January 5, 2018 00:51

veronicagg suggested changes Jan 5, 2018

View reviewed changes

Add live validation

8664d10

vladbarosan force-pushed the addLiveValidation branch from 3e21aa0 to a64b73b Compare January 8, 2018 18:56

vladbarosan closed this Jan 8, 2018

vladbarosan reopened this Jan 8, 2018

veronicagg reviewed Jan 9, 2018

View reviewed changes

veronicagg approved these changes Jan 9, 2018

View reviewed changes

Address PR comments

ef3bd27

vladbarosan force-pushed the addLiveValidation branch from a64b73b to ef3bd27 Compare January 9, 2018 18:23

vladbarosan merged commit 7f2c040 into Azure:master Jan 9, 2018

Comments

Conversation

vladbarosan commented Jan 4, 2018

Uh oh!

vladbarosan commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veronicagg commented Jan 4, 2018

Uh oh!

lmazuel commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladbarosan commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladbarosan Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladbarosan Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladbarosan Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veronicagg Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladbarosan commented Jan 4, 2018 •

edited

Loading

lmazuel commented Jan 4, 2018 •

edited

Loading

vladbarosan commented Jan 5, 2018 •

edited

Loading

vladbarosan Jan 5, 2018 •

edited

Loading

vladbarosan Jan 5, 2018 •

edited

Loading

vladbarosan Jan 5, 2018 •

edited

Loading

veronicagg Jan 9, 2018 •

edited

Loading