Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

vishrutshah
Copy link
Contributor

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.

@vishrutshah vishrutshah self-assigned this Oct 3, 2017
console.dir(e, { depth: null, colors: true });
}
// Do not catch the JSON parse error
jsonResult = JSON.parse(resultString);
Copy link
Contributor

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?

Copy link
Contributor Author

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

  1. The CI Job would be red
  2. It shows something like https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957467#L590 in the log
  3. 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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishrutshah thanks!
#1 Yes, great, that's what we want for the bot job.
Between #2 and #3 I think it'd choose #3, reason is because the error can get lost, so having it write the sentence that you purposefully chose may be helpful.
I guess, let's try and see, but we want at least #1 :)

@salameer
Copy link
Member

Hey Guys are we planning on blocking merge on linter errors?

Adding @kirthik as FYI for this

@veronicagg
Copy link
Contributor

@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 vladbarosan self-assigned this Nov 2, 2017
@marstr marstr changed the base branch from current to master December 28, 2017 18:01
@salameer
Copy link
Member

@vladbarosan I think this is what we're talking about here

@salameer
Copy link
Member

@sarangan12 this is yours for what we talked about the other day

@salameer
Copy link
Member

adding @bsiegel

@bsiegel bsiegel self-assigned this Mar 30, 2018
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@sarangan12
Copy link
Member

The tool has been modified a lot and this no longer applies. Closing this

@sarangan12 sarangan12 closed this Sep 6, 2018
@AutorestCI
Copy link

AutorestCI commented Sep 6, 2018

Automation for azure-sdk-for-python

Unable to detect any generation context from this PR.

@AutorestCI
Copy link

AutorestCI commented Sep 6, 2018

Automation for azure-sdk-for-go

Unable to detect any generation context from this PR.

@AutorestCI
Copy link

AutorestCI commented Sep 6, 2018

Automation for azure-sdk-for-node

Unable to detect any generation context from this PR.

@AutorestCI
Copy link

AutorestCI commented Sep 6, 2018

Automation for azure-sdk-for-java

Unable to detect any generation context from this PR.

@AutorestCI
Copy link

AutorestCI commented Sep 6, 2018

Automation for azure-sdk-for-ruby

Unable to detect any generation context from this PR.

mccleanp pushed a commit that referenced this pull request Mar 23, 2022
[footprintmonitoring] Make provisioning state read-only, change example names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants