-
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
[Travis-CI] Run breaking change tool in the CI #1456
Conversation
getFilesChangedInPR should not return package.json
This reverts commit e2db85c.
@amarzavery @veronicagg Feel free to review this PR as you get chance. Thanks! |
test/util/utils.js
Outdated
@@ -73,6 +73,22 @@ exports.getTargetBranch = function getTargetBranch() { | |||
}; | |||
|
|||
/** | |||
* Checkouts targetBranch | |||
* | |||
* @param {string} swaggerPath Path to the swagger specification 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.
Copy pasta ? 😳
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.
left couple of comments, generally looks good to me.
scripts/breaking-change.js
Outdated
} | ||
|
||
let swaggersToProcess = utils.getFilesChangedInPR(); | ||
if (!isRunningInTraviCI) { |
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.
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....."
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.
👍
test/util/utils.js
Outdated
@@ -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) { |
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 the purpose of the addition to the condition 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.
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.
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.
sorry meant to indicate "request changes" before :)
@veronicagg I've incorporated the review feedback. Feel free to re-review as you get chance. Thanks! |
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: File: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
@@ -1711,11 +1711,6 @@ | |||
"type": "string", | |||
"x-ms-client-name": "ContentLanguage", | |||
"description": "The response header override for content language." | |||
}, | |||
"rsct": { |
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.
why do you have this change in the PR?
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 to just make sure that tool runs and outputs things in CI. I'll be reverting that commit as soon as review completes :)
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.
yes, please, let's make sure changes to the spec do not get merged.
@@ -583,7 +583,7 @@ | |||
}, | |||
"capacity": { | |||
"type": "integer", | |||
"format": "int32", | |||
"format": "int64", |
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.
why do we have this change in the PR?
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.
same as above
scripts/breaking-change.js
Outdated
console.log(`Processing via AutoRest...`); | ||
|
||
let outputFileNameWithExt = swaggerPath.split('/').pop(); | ||
let outputFileNameWithoutExt = outputFileNameWithExt.split('.')[0]; |
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.
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.
console.log(`Executing : ${autoRestCmd}`); | ||
|
||
try { | ||
let result = execSync(`${autoRestCmd}`, { encoding: 'utf8', maxBuffer: 1024 * 1024 * 64 }); |
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.
will this value for maxBuffer: 1024 * 1024 * 64 be sufficient enough ? Wanted to know how you came up with this value?
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 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.
} | ||
|
||
//main function | ||
async function runScript() { |
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.
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
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 it's moved to 8.0 already as we used async
before :)
scripts/breaking-change.js
Outdated
console.dir(resolvedMapForNewSpecs); | ||
|
||
for (const swagger of swaggersToProcess) { | ||
let outputFileNameWithExt = swagger.split('/').pop(); |
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.
using the path module will be a better option.
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, updated to let outputFileNameWithExt = path.basename(swagger)
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: File: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
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, please make sure not to commit changes to the storage spec.
scripts/breaking-change.js
Outdated
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 |
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.
small typo in comment... "update"?
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. typo fixed
@@ -1711,11 +1711,6 @@ | |||
"type": "string", | |||
"x-ms-client-name": "ContentLanguage", | |||
"description": "The response header override for content language." | |||
}, | |||
"rsct": { |
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.
yes, please, let's make sure changes to the spec do not get merged.
This reverts commit 95aed00.
Adding some differentiator for CI output
Thanks everyone for the review!! |
No modification for AutorestCI/azure-sdk-for-python |
No modification for AutorestCI/azure-sdk-for-ruby |
No modification for AutorestCI/azure-sdk-for-node |
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: