-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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] Azure PR Bot CI job must fail on the JSON.parse error from linter output #1792
Conversation
console.dir(e, { depth: null, colors: true }); | ||
} | ||
// Do not catch the JSON parse error | ||
jsonResult = JSON.parse(resultString); |
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 will happen when this throws? will the message in the PR just not show up? with the job showing a failure?
From the bot output perspective is the exception readable? or would it be better to keep the message you had and either re-throw the exception or try to update the exit code of the script?
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.
That's a good point..With this changes
- The CI Job would be red
- It shows something like https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L590 in the log
- If we would prefer to catch and re-throw there'd be one more line like https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L588
We can catch and re-throw if you think that'd be better on the CI. The only thought I was having was on re-throw, what'd be the best way to make this visible :) What do you think ?
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.
Hey Guys are we planning on blocking merge on linter errors? Adding @kirthik as FYI for this |
@salameer this is not tracking making linter a blocker, this is about the bot job and how it reports when there are linter errors. The problem was that if the bot was not able to parse the output given to it by the linter, it would report green, which would be misleading. |
@vladbarosan I think this is what we're talking about here |
@sarangan12 this is yours for what we talked about the other day |
adding @bsiegel |
Can one of the admins verify this patch? |
The tool has been modified a lot and this no longer applies. Closing this |
Automation for azure-sdk-for-pythonUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-goUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-nodeUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-javaUnable to detect any generation context from this PR. |
Automation for azure-sdk-for-rubyUnable to detect any generation context from this PR. |
[footprintmonitoring] Make provisioning state read-only, change example names.
As of today we catch the JSON.parse error on the linter output for the Azure Bot to report status. With this we are having trouble that Azure Bot reports there were no errors but that is not true.
Example PR: #1788 (comment)
CI Bot Error: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L607
By not catching this error, we'll fail the CI job and make Azure Bot not report the invalid report.