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

Don't check for same user ID on comment when running as a GitHub App #525

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Don't check for same user ID on comment when running as a GitHub App #525

merged 2 commits into from
Mar 14, 2018

Conversation

tibdex
Copy link
Contributor

@tibdex tibdex commented Mar 1, 2018

I think the line was wrongly changed here.
The consequence is that when running as a GitHub App, Danger will keep creating new comments instead of updating the existing one.

@orta
Copy link
Member

orta commented Mar 1, 2018

For peril? You should be setting the user ID otherwise it'll delete Danger messages that came from the CI as well

This should be pretty easy to fix in Peril - just need to spend some time on danger/peril#179

@tibdex
Copy link
Contributor Author

tibdex commented Mar 1, 2018

I'm not using Peril.

I use Danger as part of our CircleCI build.
But I'm working in an organization and we prefer using GitHub Apps instead of personal access tokens or bot accounts. We find it cleaner as it's easy to install the GitHub App on a set of internal repositories and it doesn't take an additional seat on our organization like a bot account would.

PS: The latest Travis CI build failed but the previous one was fine. The only difference is the addition of an entry in the changelog. Can you relaunch the build and see if it passes this time 😉?

@tibdex
Copy link
Contributor Author

tibdex commented Mar 14, 2018

I think the line was wrongly changed here.

Hi @orta, as I said in my first message, I think this PR is valuable even for non-Peril users.

@orta
Copy link
Member

orta commented Mar 14, 2018

Oh yeah, sorry, yeah - I think this makes sense 👍

@orta
Copy link
Member

orta commented Mar 14, 2018

OK, that green'd

@orta orta merged commit a2966c4 into danger:master Mar 14, 2018
@ggajews
Copy link

ggajews commented Mar 19, 2019

@tibdex can you write how did you guys manage to run danger with the Github App? I'm struggling with that for a while. Do you get your app token before initializing danger?
Sorry for commenting on this old issue.

@tibdex
Copy link
Contributor Author

tibdex commented Mar 19, 2019

We do it like this:

// Danger requires a DANGER_GITHUB_API_TOKEN environment property to work.
// Since we use a GitHub App to interact with the repository, there is an intermediate
// step which is to convert the GitHub App private key to an access token.
// We use fetch-github-app for that.

'use strict';

require('loud-rejection/register');

const envalid = require('envalid');
const execa = require('execa');
const fetchGithubApp = require('fetch-github-app');
const findYarnWorkspaceRoot = require('find-yarn-workspace-root');

const env = envalid.cleanEnv(
  process.env, // eslint-disable-line  no-process-env
  {
    DANGER_GITHUB_APP_ID: envalid.num({
      desc:
        'Can be found in the "GitHub Apps" section of the organization settings.',
    }),
    // Using base64 encoding in order not to deal with cumbersome EOL escaping.
    // eslint-disable-next-line id-match
    DANGER_GITHUB_APP_PEM_BASE64_ENCODED: envalid.makeValidator(str => {
      const privateKey = Buffer.from(str, 'base64').toString('utf8');
      if (
        /-----BEGIN RSA PRIVATE KEY-----[\s\S]+-----END RSA PRIVATE KEY-----/m.test(
          privateKey,
        )
      ) {
        return privateKey;
      }
      throw new Error('invalid GitHub App RSA private key');
    })({
      docs:
        'https://developer.github.com/apps/building-integrations/setting-up-and-registering-github-apps/registering-github-apps/#generating-a-private-key',
    }),
    DANGER_GITHUB_INSTALLATION_ID: envalid.num({
      desc:
        'Can be found in the "Installed GitHub Apps" section of the organization settings',
    }),
  },
  {strict: true},
);

fetchGithubApp({
  appId: env.DANGER_GITHUB_APP_ID,
  // Using base64 encoding in order not to deal with cumbersome EOL escaping.
  appPrivateKey: env.DANGER_GITHUB_APP_PEM_BASE64_ENCODED,
  installationId: env.DANGER_GITHUB_INSTALLATION_ID,
  userAgent: 'activeviam',
}).then(({installationAccessToken}) =>
  execa(
    'danger',
    ['--dangerfile', 'danger/dangerfile.js', ...process.argv.slice(2)],
    {
      // Extend the environment of the child process with the access token.
      env: {DANGER_GITHUB_API_TOKEN: installationAccessToken},
      // To display Danger output in the console.
      stdio: 'inherit',
      cwd: findYarnWorkspaceRoot(),
    },
  ),
);

Instead of fetch-github-app you should be able to use https://www.npmjs.com/package/@octokit/app

@ggajews
Copy link

ggajews commented Mar 20, 2019

Sweet, thanks a lot!

@orta
Copy link
Member

orta commented Mar 20, 2019

I wonder if that could be moved into a custom command in danger-js?

@hemp
Copy link

hemp commented Nov 9, 2020

Thanks @tibdex , that code above was useful.

Currently using @octokit/auth-app https://www.npmjs.com/package/@octokit/auth-app in place of fetch-github-app since it has been since archived.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants