-
Notifications
You must be signed in to change notification settings - Fork 632
fix: Allow Github.com auth when github-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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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, the options
should include silent
.
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?
const githubOptions = { ...options };
if (enterprise && !enterprise.canceled && this.isAuthenticated(AuthProvider.githubEnterprise)) {
githubOptions.silent = true;
}
const github = await this.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 the isAuthenticated
check is needed, but otherwise it looks good.
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. |
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 set per 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