- Notifications
You must be signed in to change notification settings - Fork630
fix: Allow Github.com auth whengithub-enterprise.uri
is set#7002
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@alexr00 could I get a review? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the PR! I understand the motivation, but I think we need one change.
src/github/credentials.ts Outdated
@@ -221,6 +221,7 @@ export class CredentialStore extends Disposable { | |||
} | |||
private async doCreate(options: vscode.AuthenticationGetSessionOptions, additionalScopes: boolean = false): Promise<AuthResult> { | |||
const github = await this.initialize(AuthProvider.github, options, additionalScopes ? SCOPES_WITH_ADDITIONAL : undefined, additionalScopes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will cause a badge to be shown on the Accounts menu, prompting for github.com auth, even when enterprise auth is successful. If enterprise auth has succeeded, theoptions
should includesilent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the feedback!
What do you think of this approach?
constgithubOptions={ ...options};if(enterprise&&!enterprise.canceled&&this.isAuthenticated(AuthProvider.githubEnterprise)){githubOptions.silent=true;}constgithub=awaitthis.initialize(AuthProvider.github,githubOptions,additionalScopes ?SCOPES_WITH_ADDITIONAL :undefined,additionalScopes);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know that theisAuthenticated
check is needed, but otherwise it looks good.
alexr00 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
ad89f17
intomicrosoft:mainUh oh!
There was an error while loading.Please reload this page.
Problem
Since version
0.110.0
, setting the following configuration:causes the GitHub.com authentication flow to be skipped entirely. This results in:
github.com
Github.com
repos breaking silentlyFor many developers (including myself), contributing to both
github.com
and a company-hosted GitHub Enterprise is common.Even though the config can be setper workspace, it can be tedious to maintain across multiple repositories.
And, in practice, setting
github-enterprise.uri
in user settings makes more sense. But this intentionally disablesgithub.com
functionality with the current logic.What does change?
This PR updates
doCreate
so that GitHub.com is initialized even though the enterprise URI is configured.This mirrors pre-
0.110.0
behavior.Before
After