Customise GHE actions bot user ID #1404
Conversation
|
Hey @orta 👋 When you have a moment, could you review this PR, please? |
source/platforms/github/GitHubAPI.ts
Outdated
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const userID = await api.getUserID() | ||
| expect(userID).toBe(1) | ||
| }) | ||
| }) | ||
|
|
||
| describe("Allows setting GHE_ACTIONS_BOT_USER_ID env variable", () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also, of course, once we need to document this env-var in appropriate places. Probably with a section for “GitHub Enterprise”
There was a problem hiding this comment.
Yep makes sense. I have update the name of the env variable and added some docs. Please let me know if I missed something.
| @@ -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 () => { | |||
There was a problem hiding this comment.
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.idis set, it returns that UserID AndGITHUB_WORKFLOW, GHE_ACTIONS_BOT_USER_ID, PERIL_BOT_USER = undefined— this test case might be hard, sorry! PERIL_BOT_USERif nobody is logged in (GHE_ACTIONS_BOT_USER_IDis unset, butGITHUB_WORKFLOWis set, andPERIL_BOT_USERis set,info.idreturns undefined)41898282ifPERIL_BOT_USERis unset,GHE_ACTIONS_BOT_USER_IDis unset, butGITHUB_WORKFLOWis set, andinfo.idwould be undefined.- The test case you’ve already added below
"Allows setting GHE_ACTIONS_BOT_USER_ID env variable"
There was a problem hiding this comment.
I think the tests should now be updated to cover all scenarios.
dimitar-hristov
force-pushed
the
ghe-actions-app-id
branch
from
9754bbb
to
f918662
Compare
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. 🙂 |
|
I think this is good to go - thanks both of you 👍🏻 |
Context
Changes
DANGER_GHE_ACTIONS_BOT_USER_ID.