Customise GHE actions bot user ID #1404

dimitar-hristov · 2023-09-10T09:50:21Z

Context

In GitHub actions, the user ID of the bot is hardcoded for github.com without an option to overwrite it for GitHub Enterprise. As a result when called here and here to update the PR comments, it fails to find the comments.

Changes

Adds logic to customise DANGER_GHE_ACTIONS_BOT_USER_ID .

dimitar-hristov · 2023-09-10T09:57:43Z

Hey @orta 👋 When you have a moment, could you review this PR, please?

fbartho

No objections to the purpose of this PR — just gotta make sure it doesn’t break :P — Happy to take another look once you’ve ensured it won’t!

fbartho · 2023-09-10T17:38:42Z

source/platforms/github/GitHubAPI.ts

      // This is the user.id of the github-actions app (https://github.com/apps/github-actions)
      // that is used to comment when using danger in a GitHub Action
      // with GITHUB_TOKEN (https://help.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token)
      return 41898282
    }

+    const info = await this.getUserInfo()


This reorganization will cause a breaking behavior change even if GHE_ACTIONS_BOT_USER_ID is unset.

Originally, if this.getUserInfo returns a logged in user then it would immediately return with info.id. Now, it will first check if GITHUB_WORKFLOW is set (and it’s always set on Github, I believe), and so it will always return a hardcoded ID

This would break our codebase, as we set an env-var, I believe DANGER_GITHUB_TOKEN or similar (not at my desk) to log in to a bot account with a PAT. This code-change would only allow comments from the GitHub-actions app-id of 41898282.

I see, now it makes more sense 😄 I think I found the env var you referred to - it seems to be DANGER_GITHUB_API_TOKEN.

I reverted the change, but also added a logic around it to ensure we call the GitHub API for the user only if the DANGER_GITHUB_API_TOKEN is set. Otherwise, I am not sure if we want to make the call, when there isn't a configured user. Do you think that makes sense?

Unfortunately, the regular GITHUB_TOKEN might be a valid logged in user so it’s not sufficient to only focuse on DANGER_GITHUB_API_TOKEN, I think! — As I understand it, that’s why we call the getUserInfo API — so we figure out the permissions on the attached token — figure out if it’s a real user or what exactly.

Hmm, ok, I reverted that part of my change. I still did the tests for completeness, but i don't think they are relevant to my change anymore.

The test that check if the configured token is a user is a bit weird, but I think it makes sense as it check that we have called the API with the correct creds (the ones used when creating the GitHub instance). And ensure that it correctly returns the mocked user ID.

Let me know what you think.

fbartho · 2023-09-10T17:40:42Z

source/platforms/github/_tests/_github_api.test.ts

      const userID = await api.getUserID()
      expect(userID).toBe(1)
    })
  })
+
+  describe("Allows setting GHE_ACTIONS_BOT_USER_ID env variable", () => {


GHE_ACTIONS_BOT_USER_ID — this is a custom environment variable, right? (not set by github, set by the user).

The naming convention for DangerJS custom environment variables is to prefix it with DANGER_, so in this case, I would name it something like DANGER_GHE_ACTIONS_BOT_USER_ID

Also, of course, once we need to document this env-var in appropriate places. Probably with a section for “GitHub Enterprise”

Yep makes sense. I have update the name of the env variable and added some docs. Please let me know if I missed something.

fbartho · 2023-09-10T17:50:48Z

source/platforms/github/_tests/_github_api.test.ts

@@ -363,9 +363,26 @@ describe("Peril", () => {
      delete process.env.PERIL_BOT_USER_ID
    })

-    it("Makes getUserId return undefined", async () => {
+    it("Makes getUserId return PERIL_BOT_USER_ID", async () => {


I believe shouldn’t always return PERIL_BOT_USER here, instead we need something like these test-cases.:

A test case that matches the old test name where all vars are unset, GITHUB_WORKFLOW, GHE_ACTIONS_BOT_USER_ID, PERIL_BOT_USER = undefined

A test case that confirms that when info.id is set, it returns that UserID And GITHUB_WORKFLOW, GHE_ACTIONS_BOT_USER_ID, PERIL_BOT_USER = undefined — this test case might be hard, sorry!

PERIL_BOT_USER if nobody is logged in (GHE_ACTIONS_BOT_USER_ID is unset, but GITHUB_WORKFLOW is set, and PERIL_BOT_USER is set, info.id returns undefined)

41898282 if PERIL_BOT_USER is unset, GHE_ACTIONS_BOT_USER_ID is unset, but GITHUB_WORKFLOW is set, and info.id would be undefined.

The test case you’ve already added below"Allows setting GHE_ACTIONS_BOT_USER_ID env variable"

I think the tests should now be updated to cover all scenarios.

dimitar-hristov · 2023-09-10T22:48:34Z

No objections to the purpose of this PR — just gotta make sure it doesn’t break :P — Happy to take another look once you’ve ensured it won’t!

Thanks for the quick response, @fbartho! I think all comments should now be addressed. Please have a look at the PR again when you have a moment and let me know if it is better. 🙂

orta · 2023-12-07T17:07:04Z

I think this is good to go - thanks both of you 👍🏻

Customise GHE actions bot user ID

c9612ec

dimitar-hristov marked this pull request as ready for review September 10, 2023 09:55

fbartho requested changes Sep 10, 2023

View reviewed changes

fbartho added the [platform] GitHub label Sep 10, 2023

dimitar-hristov added 3 commits September 10, 2023 22:44

Address review comments

2d07b76

Add DANGER_ prefix to new env var

7bae2d3

Add docs

f918662

dimitar-hristov force-pushed the ghe-actions-app-id branch from 9754bbb to f918662 Compare September 10, 2023 22:30

Fix typo

772c14d

Revert change around getUserInfo and unsure it is tested

87290e7

orta merged commit 266c485 into danger:main Dec 7, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customise GHE actions bot user ID #1404

Customise GHE actions bot user ID #1404

dimitar-hristov commented Sep 10, 2023 •

edited

Loading

dimitar-hristov commented Sep 10, 2023

fbartho left a comment

fbartho Sep 10, 2023

dimitar-hristov Sep 10, 2023

fbartho Sep 11, 2023

dimitar-hristov Sep 11, 2023

fbartho Sep 10, 2023

fbartho Sep 10, 2023

dimitar-hristov Sep 10, 2023

fbartho Sep 10, 2023 •

edited

Loading

dimitar-hristov Sep 10, 2023

dimitar-hristov commented Sep 10, 2023

orta commented Dec 7, 2023

Customise GHE actions bot user ID #1404

Customise GHE actions bot user ID #1404

Conversation

dimitar-hristov commented Sep 10, 2023 • edited Loading

Context

Changes

dimitar-hristov commented Sep 10, 2023

fbartho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbartho Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitar-hristov commented Sep 10, 2023

orta commented Dec 7, 2023

dimitar-hristov commented Sep 10, 2023 •

edited

Loading

fbartho Sep 10, 2023 •

edited

Loading