[Travis-CI] Run breaking change tool in the CI #1456

vishrutshah · 2017-07-21T16:54:32Z

Running breaking change tool in the CI on the changed swagger json files.

Issue: Azure/openapi-diff#51
Successful Build: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/256216203#L688

TODO:

Disabling the other CI modes to speedup the build for test
Updating the storage files to test
Enable the other CI modes
Make sure to NOT check-in the swagger changes
Revert the test changes

getFilesChangedInPR should not return package.json

This reverts commit e2db85c.

vishrutshah · 2017-07-21T22:01:27Z

@amarzavery @veronicagg Feel free to review this PR as you get chance. Thanks!

vishrutshah · 2017-07-21T22:02:27Z

test/util/utils.js

@@ -73,6 +73,22 @@ exports.getTargetBranch = function getTargetBranch() {
 };

 /**
+ * Checkouts targetBranch
+ *
+ * @param {string} swaggerPath Path to the swagger specification file.


Copy pasta ? 😳

veronicagg

left couple of comments, generally looks good to me.

veronicagg · 2017-07-26T22:52:56Z

scripts/breaking-change.js

+  }
+
+  let swaggersToProcess = utils.getFilesChangedInPR();
+  if (!isRunningInTraviCI) {


is this is applicable only for a local test and would need to be modified (as paths are pointing to your repo), I'd recommend commenting the whole section with a comment like "For debugging in your repo, un-comment the following section and....."

veronicagg · 2017-07-26T22:55:58Z

test/util/utils.js

@@ -203,7 +219,7 @@ exports.getFilesChangedInPR = function getFilesChangedInPR() {
      console.log('>>>>> Files changed in this PR are as follows:')
      console.log(filesChanged);
      swaggerFilesInPR = filesChanged.split('\n').filter(function (item) {
-        if (item.match(/.*json$/ig) == null) {
+        if (item.match(/.*json$/ig) == null || item.match(/package.json$/ig) != null) {


what's the purpose of the addition to the condition here?

As in this PR I've changed package.json it is coming into array of changed swaggers in PR but it should not. So I added that condition but now i think i may have better condition.

Look for files those have specification into their path instead looking for all .json & then ignoring.

veronicagg

sorry meant to indicate "request changes" before :)

This reverts commit c1fb793.

vishrutshah · 2017-07-28T18:53:17Z

@veronicagg I've incorporated the review feedback. Feel free to re-review as you get chance. Thanks!

azuresdkciprbot · 2017-07-28T18:56:13Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/network/resource-manager/readme.md
Before the PR: Warning(s): 151 Error(s): 84
After the PR: Warning(s): 151 Error(s): 84

File: specification/storage/resource-manager/readme.md
Before the PR: Warning(s): 11 Error(s): 1
After the PR: Warning(s): 11 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

amarzavery · 2017-07-28T20:36:08Z

specification/storage/resource-manager/Microsoft.Storage/2017-06-01/storage.json

@@ -1711,11 +1711,6 @@
          "type": "string",
          "x-ms-client-name": "ContentLanguage",
          "description": "The response header override for content language."
-        },
-        "rsct": {


why do you have this change in the PR?

This is to just make sure that tool runs and outputs things in CI. I'll be reverting that commit as soon as review completes :)

yes, please, let's make sure changes to the spec do not get merged.

amarzavery · 2017-07-28T20:36:26Z

specification/network/resource-manager/Microsoft.Network/2017-06-01/applicationGateway.json

@@ -583,7 +583,7 @@
        },
        "capacity": {
          "type": "integer",
-          "format": "int32",
+          "format": "int64",


why do we have this change in the PR?

same as above

amarzavery · 2017-07-28T20:41:27Z

scripts/breaking-change.js

+  console.log(`Processing via AutoRest...`);
+
+  let outputFileNameWithExt = swaggerPath.split('/').pop();
+  let outputFileNameWithoutExt = outputFileNameWithExt.split('.')[0];


Rather than using the logic on line 53-54, please use the path module available in node.js.

path.basename()

path.dirname()

path.extname()

should be useful for you.

amarzavery · 2017-07-28T20:43:27Z

scripts/breaking-change.js

+  console.log(`Executing : ${autoRestCmd}`);
+
+  try {
+    let result = execSync(`${autoRestCmd}`, { encoding: 'utf8', maxBuffer: 1024 * 1024 * 64 });


will this value for maxBuffer: 1024 * 1024 * 64 be sufficient enough ? Wanted to know how you came up with this value?

not sure as I derived it from linter https://github.com/Azure/azure-rest-api-specs/blob/current/test/linter.js#L20

I think idea was to keep something to make sure we see when things goes over some number.

amarzavery · 2017-07-28T20:45:16Z

scripts/breaking-change.js

+}
+
+//main function
+async function runScript() {


by using this functionality you are expecting everyone to use the v8.x (an unstable version) on their box. Make sure that travis ci also picks up 8.x

Yup it's moved to 8.0 already as we used async before :)

amarzavery · 2017-07-28T20:46:08Z

scripts/breaking-change.js

+  console.dir(resolvedMapForNewSpecs);
+
+  for (const swagger of swaggersToProcess) {
+    let outputFileNameWithExt = swagger.split('/').pop();


using the path module will be a better option.

Sure, updated to let outputFileNameWithExt = path.basename(swagger)

azuresdkciprbot · 2017-07-28T22:24:11Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/network/resource-manager/readme.md
Before the PR: Warning(s): 148 Error(s): 83
After the PR: Warning(s): 148 Error(s): 83

File: specification/storage/resource-manager/readme.md
Before the PR: Warning(s): 11 Error(s): 1
After the PR: Warning(s): 11 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

veronicagg

looks good, please make sure not to commit changes to the storage spec.

veronicagg · 2017-08-01T20:41:21Z

scripts/breaking-change.js

+    let result = execSync(`${autoRestCmd}`, { encoding: 'utf8', maxBuffer: 1024 * 1024 * 64 });
+    resolvedMapForNewSpecs[outputFileNameWithExt] = path.join(outputFolder, outputFileNameWithExt);
+  } catch (err) {
+    // Do not updage map in case of errors


small typo in comment... "update"?

Yup. typo fixed

veronicagg · 2017-08-01T20:46:53Z

specification/storage/resource-manager/Microsoft.Storage/2017-06-01/storage.json

@@ -1711,11 +1711,6 @@
          "type": "string",
          "x-ms-client-name": "ContentLanguage",
          "description": "The response header override for content language."
-        },
-        "rsct": {


yes, please, let's make sure changes to the spec do not get merged.

This reverts commit 95aed00.

Adding some differentiator for CI output

vishrutshah · 2017-08-01T22:56:12Z

Thanks everyone for the review!!

AutorestCI · 2017-08-01T23:00:16Z

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

AutorestCI · 2017-08-01T23:00:19Z

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

AutorestCI · 2017-08-01T23:00:26Z

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

msftclas added the cla-already-signed label Jul 21, 2017

vishrutshah self-assigned this Jul 21, 2017

vishrutshah force-pushed the oad-in-ci branch from 469172e to fa8a9f9 Compare July 21, 2017 17:06

Run breaking change tool in the CI

32bef2f

getFilesChangedInPR should not return package.json

vishrutshah force-pushed the oad-in-ci branch from 8868634 to 32bef2f Compare July 21, 2017 20:51

vishrutshah requested review from amarzavery and veronicagg July 21, 2017 20:51

vishrutshah added 2 commits July 21, 2017 13:53

Update two swagger to test in CI

e2db85c

Revert "Update two swagger to test in CI"

c1fb793

This reverts commit e2db85c.

vishrutshah changed the title ~~[WIP] [Travis-CI] Run breaking change tool in the CI~~ [Travis-CI] Run breaking change tool in the CI Jul 21, 2017

vishrutshah commented Jul 21, 2017

View reviewed changes

veronicagg approved these changes Jul 26, 2017

View reviewed changes

veronicagg suggested changes Jul 26, 2017

View reviewed changes

vishrutshah added 3 commits July 28, 2017 10:18

Addressing code review comments

af9f9f7

Revert "Revert "Update two swagger to test in CI""

95aed00

This reverts commit c1fb793.

Path that contains specification word

26a82fe

amarzavery reviewed Jul 28, 2017

View reviewed changes

Incorporating code review comments

03a0c5a

vishrutshah force-pushed the oad-in-ci branch from e36f4b2 to 03a0c5a Compare July 28, 2017 21:59

veronicagg approved these changes Aug 1, 2017

View reviewed changes

vishrutshah added 2 commits August 1, 2017 15:29

Revert "Revert "Revert "Update two swagger to test in CI"""

e4c0492

This reverts commit 95aed00.

Typo fixed

7ec0724

Adding some differentiator for CI output

vishrutshah force-pushed the oad-in-ci branch from 5a0b74b to 7ec0724 Compare August 1, 2017 22:44

vishrutshah merged commit 521c0e6 into Azure:current Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Travis-CI] Run breaking change tool in the CI #1456

[Travis-CI] Run breaking change tool in the CI #1456

vishrutshah commented Jul 21, 2017 •

edited

Loading

vishrutshah commented Jul 21, 2017

vishrutshah Jul 21, 2017

veronicagg left a comment

veronicagg Jul 26, 2017

vishrutshah Jul 28, 2017

veronicagg Jul 26, 2017

vishrutshah Jul 28, 2017

veronicagg left a comment

vishrutshah commented Jul 28, 2017

azuresdkciprbot commented Jul 28, 2017

amarzavery Jul 28, 2017

vishrutshah Jul 28, 2017

veronicagg Aug 1, 2017

amarzavery Jul 28, 2017

vishrutshah Jul 28, 2017

amarzavery Jul 28, 2017

amarzavery Jul 28, 2017

vishrutshah Jul 28, 2017

amarzavery Jul 28, 2017

vishrutshah Jul 28, 2017

amarzavery Jul 28, 2017

vishrutshah Jul 28, 2017

azuresdkciprbot commented Jul 28, 2017

veronicagg left a comment

veronicagg Aug 1, 2017

vishrutshah Aug 1, 2017

veronicagg Aug 1, 2017

vishrutshah commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

[Travis-CI] Run breaking change tool in the CI #1456

[Travis-CI] Run breaking change tool in the CI #1456

Conversation

vishrutshah commented Jul 21, 2017 • edited Loading

vishrutshah commented Jul 21, 2017

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg left a comment

Choose a reason for hiding this comment

vishrutshah commented Jul 28, 2017

azuresdkciprbot commented Jul 28, 2017

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

azuresdkciprbot commented Jul 28, 2017

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

Choose a reason for hiding this comment

vishrutshah commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

AutorestCI commented Aug 1, 2017

vishrutshah commented Jul 21, 2017 •

edited

Loading