Add GitHub authentication#874
Conversation
Adds a command to authenticate to GitHub. This doesn't do anything yet, but will be helpful in future so we can use the GitHub API. I had to update to VS Code 1.48.0 to use the authentication API, so there are some unrelated changes to make existing code compatible.
Fixes the parsing errors: `Cannot read property 'map' of undefined`
(+ disabled a few rules that aren't important)
(since we've disabled the "ban Function type" linting check in test files)
aeisenberg
left a comment
There was a problem hiding this comment.
Looks sensible. A few comments.
robertbrignull
left a comment
There was a problem hiding this comment.
Thought I'd have a read through of this too, so I can learn a bit about how the vscode extension works too.
|
|
||
| async initialize(context: vscode.ExtensionContext): Promise<void> { | ||
| this.registerListeners(context); | ||
| this.setOctokit(); |
There was a problem hiding this comment.
This line needs an await.
You may find it useful to add the "@typescript-eslint/no-floating-promises": "error" linting rule, though I tried it and there are a lot of cases caused by async logging calls, so probably not something to look at in this PR.
There was a problem hiding this comment.
That's a good idea. We can use the ignoreVoid: true flag to allow the explicit ignoring of promises. It's a bit of a change to make the switchover, but probably worth it. I'll look at it later today.
aeisenberg
left a comment
There was a problem hiding this comment.
Some stylistic comments that you can take or leave.
aeisenberg
left a comment
There was a problem hiding this comment.
Comment on line 24 is important, and the one on 32 isn't.
|
Trying this out locally. And I realize you need to add the |
|
And, is there a way to log out once you log in? There doesn't need to be a command for this, but I would like to be able to test different workflows and make sure that there's no way to break things. |
I've been using the "Accounts" menu in the sidebar to manage access or sign out:
|
|
Thanks... Here's an error path. I'm not sure if there's much you can do about it since the auth flow is a bit error prone in some ways anyway:
Now, you are not authorized and repeatedly trying to run the authorize command does not work. Again, this might be a bug in the vscode API, but something to explore. |
Hmm yes, that is annoying... I've seen the same behaviour with other extensions (for example when signing into Live Share), but it's not ideal in any case 🤔 I've found a few ways to get out of the non-authorized state (e.g. by clicking "Signing into GitHub..." in the notification bar at the bottom of VS Code, or clicking "Accounts > Sign in to use CodeQL" from the sidebar. So it's probably OK for now, but we might want to add some kind of warning/notification for users when this is eventually ready to go live. |
|
(PS: Targetting this PR against a new development branch, since we don't want to merge to |
aeisenberg
left a comment
There was a problem hiding this comment.
Looks great. The remaining auto workflow bugs are outside the scope of this extension.
Two new "canary" commands: * GitHub authentication (from #874) * Workflow dispatch (run remote query)
Two new "canary" commands: * GitHub authentication (from github#874) * Workflow dispatch (run remote query)
Adds a command to authenticate to GitHub. This doesn't do anything yet, but will be helpful in future so we can use the GitHub API. When you run the command
CodeQL: Authenticate to GitHub, you should get a prompt to log in. If you're already logged in, a pop up shows the user you're authenticated as.The main changes here are
authentication.tsandextension.ts(mostly based on this VS Code sample). I had to update to VS Code 1.48.0 to use the authentication API, so there are lots of unrelated changes to make existing code compatible with updated packages.Tests are currently broken, presumably because we need to properly implement theEDIT: This was actually due to an outdated version ofMockExtensionContexttest class.eslint.Checklist
TODO
I'm not sure we want to merge this yet, or wait until we have a use for this authentication 🤔Merged into a separate development branch instead, so this won't be live for a while.@github/docs-content-codeqlhas been cc'd in all issues for UI or other user-facing changes made by this pull request.