-
Notifications
You must be signed in to change notification settings - Fork 226
Remove canary requirement for GitHub database download #1485
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
Changes from 4 commits
95438bb
fe5f1c4
8d5067f
ac1a97e
21c5ed0
23a0e03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,16 +76,27 @@ export class Credentials { | |
| })); | ||
| } | ||
|
|
||
| async getOctokit(): Promise<Octokit.Octokit> { | ||
| /** | ||
| * Creates or returns an instance of Octokit. | ||
| * | ||
| * @param requireAuthentication Whether the Octokit instance needs to be authentication as user. | ||
| * @returns An instance of Octokit. | ||
| */ | ||
| async getOctokit(requireAuthentication = true): Promise<Octokit.Octokit> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a case where we pass
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, not currently, but see 23a0e03 where I give a reasoning for why I left it like this |
||
| if (this.octokit) { | ||
| return this.octokit; | ||
| } | ||
|
|
||
| this.octokit = await this.createOctokit(true); | ||
| // octokit shouldn't be undefined, since we've set "createIfNone: true". | ||
| // The following block is mainly here to prevent a compiler error. | ||
| this.octokit = await this.createOctokit(requireAuthentication); | ||
|
|
||
| if (!this.octokit) { | ||
| throw new Error('Did not initialize Octokit.'); | ||
| if (requireAuthentication) { | ||
| throw new Error('Did not initialize Octokit.'); | ||
| } | ||
|
|
||
| // We don't want to set this in this.octokit because that would prevent | ||
| // authenticating when requireCredentials is true. | ||
| return new Octokit.Octokit({ retry }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be safe to store this octokit instance in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, what you have now is also fine.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not safe to store it in the instance variable. That would fail in the following scenario:
|
||
| } | ||
| return this.octokit; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1018,19 +1018,16 @@ async function activateWithInstalledDistribution( | |
| } | ||
| }; | ||
|
|
||
| // The "authenticateToGitHub" command is internal-only. | ||
| ctx.subscriptions.push( | ||
| commandRunner('codeQL.authenticateToGitHub', async () => { | ||
| if (isCanary()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this change still be here? I thought we would still only authenticate when in canary mode.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This command should never be called in non-canary mode, so this shouldn't really matter |
||
| /** | ||
| * Credentials for authenticating to GitHub. | ||
| * These are used when making API calls. | ||
| */ | ||
| const credentials = await Credentials.initialize(ctx); | ||
| const octokit = await credentials.getOctokit(); | ||
| const userInfo = await octokit.users.getAuthenticated(); | ||
| void showAndLogInformationMessage(`Authenticated to GitHub as user: ${userInfo.data.login}`); | ||
| } | ||
| /** | ||
| * Credentials for authenticating to GitHub. | ||
| * These are used when making API calls. | ||
| */ | ||
| const credentials = await Credentials.initialize(ctx); | ||
| const octokit = await credentials.getOctokit(); | ||
| const userInfo = await octokit.users.getAuthenticated(); | ||
| void showAndLogInformationMessage(`Authenticated to GitHub as user: ${userInfo.data.login}`); | ||
| })); | ||
|
|
||
| ctx.subscriptions.push( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.