Upgrade from 2.x to 3.7.17, fails to set Github status

[sgtcoolguy]

I had a working setup before using Danger.js with Github (using an access token). After upgrading from 2.x to 3.7.17, that setup broke. I modified the build to go from npx danger to npx danger ci, which I saw in the breaking changes.

However, now when danger runs, it updates the comment on the PR with the fails/warnings/messages, but the status is never updated. There's no indication of an issue in the build logs (from the output of npx danger ci).

I think the refactoring that added checks support may have broken the older issue comment/status workflow on Github.

[orta]

Is this a private repo? ( there's some related discussion in https://github.com/danger/danger/issues/996 )

[brbrr]

I've run it with --verbose flag, and indeed, the problem with token access rights:

$ /home/travis/build/Automattic/jetpack/node_modules/.bin/danger ci --verbose
Found only warnings, not failing the build.
Feedback: https://github.com/Automattic/jetpack/pull/9763#issuecomment-397276658
Could not add a commit status, the GitHub token for Danger does not have access rights.

ref: https://travis-ci.org/Automattic/jetpack/jobs/392299894

[orta]

I wonder if Danger is potentially over-catching an error in code that used to work, was refactored but now errors and is now caught and offers that message.

[sgtcoolguy]

This is a public repo that it's occurring on.

The existing access token has repo scope, I believe.

[orta]

Can you test a PR that comes from a branch instead of a fork? (looking briefly at the code it could be a base vs head repo reference issue )

[sgtcoolguy]

I'll see if I can do so this afternoon, not sure if I have my CI set up to do that.

I'm attempting to use the Checks API to see if that fixes the issue, but I don't see any activity to indicate the check was posted either after adding the app id env var to my Jenkins master.

This line looks potentially incorrect: https://github.com/danger/danger-js/blob/master/source/platforms/github/GitHubAPI.ts#L59

According to octokit, the type should be either 'token' (for access tokens), or 'app' (for the checks app token): https://github.com/octokit/rest.js#authentication - not always 'token'.

[orta]

Nice spot, I wonder if that's a problem for the checks API because in creating the check itself it sets the status ( see the bottom of https://github.com/danger/danger-js/pull/615 ) but I think it is a problem in general for people using the DSL to do things

[sgtcoolguy]

Ok, so I added the Danger.JS github app, set the APP ID env variable on Jenkins master and restarted it. So I believe this should be attempting to use the Checks workflow, not the issue/status one. I can retry again after with the older workflow by removing the env var if we need.

I've opened 2 PRs using Danger 3.7.18 with --verbose.

The first one filed as a PR from a fork is https://github.com/appcelerator/titanium_mobile/pull/10108 and it did not set the status. I do see an entry in checks, but looks like it didn't report there either? The console output was:

+ npx danger ci --verbose
Failing the build, there is 1 fail.
Feedback: https://github.com/appcelerator/titanium_mobile/pull/10108#issuecomment-397348786
Could not add a commit status, the GitHub token for Danger does not have access rights.
If the build fails, then danger will use a failing exit code.
Danger: ⅹ Failing the build, there is 1 fail.
## Failures
Tests have failed, see below for more information.
## Warnings
There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on <a href="https://jira.appcelerator.org/secure/CreateIssue!default.jspa">JIRA</a>
## Messages
:floppy_disk: <a href="https://jenkins.appcelerator.org/job/titanium-sdk/job/titanium_mobile/job/PR-10108/1/artifact/dist/mobilesdk-7.4.0.v20180614084818-osx.zip">Here's the generated SDK zipfile</a>.
## Markdowns
### Tests: 

| Classname | Name | Time | Error |
| --- | --- | --- | --- |
| android.Titanium.UI.TableView | appendSection and appendRow (TIMOB-25936) | 25.311 | Error: timeout of 5000ms exceeded |

I'm awaiting the build for the PR generated from an origin branch.

[sgtcoolguy]

@orta So I think your gut about the base vs head may be right. The PR from an origin branch did set the status properly. But the Checks tab still shows Danger.JS as queued.

The fact that it set the status and didn't update checks, even though I had the Github App id set is clearly odd, it's falling back to the old behavior. So I think maybe the auth issue I highlighted may be breaking the Checks API usage, and as a result it falls back?

[orta]

is it possible that both setup tokens are in the environment? when using the checks API I don't think it can say Could not add a commit status, the GitHub token for Danger does not have access rights. because status always returns true and so it get in here

[sgtcoolguy]

Yes, I do have both the access token and the app id set up in my CI environment.

On the PR filed from an origin branch, the danger output is this:

+ npx danger ci --verbose
Found only warnings, not failing the build.
Feedback: https://github.com/appcelerator/titanium_mobile/pull/10109#issuecomment-397374847
Danger: ✓ found only warnings, not failing the build
## Warnings
There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on <a href="https://jira.appcelerator.org/secure/CreateIssue!default.jspa">JIRA</a>
## Messages
:floppy_disk: <a href="https://jenkins.appcelerator.org/job/titanium-sdk/job/titanium_mobile/job/PR-10109/1/artifact/dist/mobilesdk-7.4.0.v20180614100919-osx.zip">Here's the generated SDK zipfile</a>.

So no, there's no mention of being unable to set status. It actually did set the commit status on the PR. What it didn't do was update the Check.

So to try and sum up:

• I have both access token and Github App ID env vars set up.
• If the PR is filed from a fork neither Checks nor the Commit Status are set (but the comment on the PR is updated).
• If the PR is filed from an origin branch, the Commit Status is set, but the Check is not updated (and again, the issue comment is updated).

[sgtcoolguy]

@orta Another follow-on. I'm trying to roll back to danger 2.x to fix the issue and have found that danger 2.1.10 also does not post a status to my Github PR, but 2.0.3 does. I have suspicion that perhaps this commit introduced the issue: https://github.com/danger/danger-js/commit/2f823641ea0ab851117285474d3963df3a92f4aa#diff-8e7d1e3a60812b856992f422cfb985c3

So I'm going to try out 2.1.5 and then 2.1.6 to see if that's true.

[sgtcoolguy]

2.0.3 works. 2.1.5 works. 2.1.10 doesn't. 2.1.6 does not.

[sgtcoolguy]

@orta ping...

[orta]

Hi, sorry, yeah - it's a shame that this is happening but it's low on my priorities to fix as I'm running a conference in a week or two, happy to help with debugging but I'm unlikely to write a solution for it anytime soon